libyang icon indicating copy to clipboard operation
libyang copied to clipboard

edit-config doesn't replace non-predicate value by value with predicate

Open optimden opened this issue 2 years ago • 1 comments

Hello! I found possible issue in libyang source code (0e1e5096fa782028f5cc852b3bc659bb14f225ee - last devel commit), which break edit-config replacement functionality for me. I have the following part of ietf-nacm in datastore:

<rule>
    <name>permit-all</name>
    <path xmlns:if="urn:ietf:params:xml:ns:yang:ietf-interfaces">/if:interfaces/if:interface</path>
</rule>

I want to change it in edit-config request by this:

<rule>
    <name>permit-all</name>
    <path xmlns:if="urn:ietf:params:xml:ns:yang:ietf-interfaces">/if:interfaces/if:interface[if:name='eth1']</path>
</rule>

My edit-config doesn't change value due instanceid comparison issue. The old value hasn't any predicates, but a new value contains one predicate ([if:name='eth1']). I got the following stacktrace during accomplish edit-config:

sr_edit_apply_replace
    |
lyd_change_term
    |
_lyd_change_term
    |
lyplg_type_compare_instanceid   <--- problem is here

Inside lyplg_type_compare_instanceid we have the following piece of code:

if ((s1->node != s2->node) || (s1->predicates && (LY_ARRAY_COUNT(s1->predicates) != LY_ARRAY_COUNT(s2->predicates)))) {
            return LY_ENOT;
        }

s1->predicates is NULL due old value hasn't predicates. But code doesn't handle difference between old and new value in such case, and function return LY_SUCCESS instead of LY_ENOT. We just skip arrays length comparison due s1->predicates condition failed. As i checked, LY_ARRAY_COUNT macro can handles NULL pointer, and return 0 in such case. So, it looks unnecessary to check s1->predicates != NULL. So, my fix was:

if ((s1->node != s2->node) || (LY_ARRAY_COUNT(s1->predicates) != LY_ARRAY_COUNT(s2->predicates))) {
            return LY_ENOT;
        }

After this old value was successfully replaced by a new one.

Please, check my suggestion, and if it's okay, i would like to create a PR in devel branch.

optimden avatar Feb 14 '24 07:02 optimden

Yes, the check seems useless and causing problems in this case.

michalvasko avatar Feb 14 '24 07:02 michalvasko