-
Notifications
You must be signed in to change notification settings - Fork 1.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Export libyang API "lyd_check_mandatory_tree" for Management framework (CVL) #5714
Export libyang API "lyd_check_mandatory_tree" for Management framework (CVL) #5714
Conversation
Sync with azure:master
Libyang patch for exporting API
Can you please help review? @zhenggen-xu , @li-pingmao , @vasant17 , @samaity, @praveen-li Thanks a lot. |
|
||
-int | ||
+API int | ||
lyd_check_mandatory_tree(struct lyd_node *root, struct ly_ctx *ctx, const struct lys_module **modules, int mod_count, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can this go to upstream?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this patch is just to export lyd_check_mandatory_tree() as API so that CVL can validate mandatory YANG nodes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah good to upstream with reason of use. Though no harm in making it an API in our code till then.
retest vs please |
retest vsimage please |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall LGTM.
|
||
-int | ||
+API int | ||
lyd_check_mandatory_tree(struct lyd_node *root, struct ly_ctx *ctx, const struct lys_module **modules, int mod_count, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah good to upstream with reason of use. Though no harm in making it an API in our code till then.
@@ -1,2 +1,3 @@ | |||
libyang.patch | |||
libyang_mgmt_framework.patch |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the Future, are we planning to put all patches related to a framework in this single file?,
If not, I feel it is better to have a file name based on the patch, for example, "API_lyd_check_mandatory_tree.patch"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we are not expecting many changes for libyang library, we have kept a single patch file for libyang changes related to management framework. Please suggest if current filename should be changed to any other generic name e.g. libyang_extension.patch or libyang_ext.patch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ext is not good, libyang_mgmt_framework.patch is fine, but if we had to add in this file in the future then we need to break it down into multiple patches with description.
That will be inline with other sonic repos, how we maintain patches. That will also help if some patches are accepted by upstream and we have to remove them in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can go ahead with this patch.
- Why I did it
Management framework (CVL) needs to call lyd_check_mandatory_tree() for validation and hence exported lyd_check_mandatory_tree() as an API.
- How I did it
Added "API" keyword before lyd_check_mandatory_tree() definition.
- How to verify it
There is no functionality code change here and no specific steps to verify it. Management framework (CVL) should be able to call this function and no patching and compilation error should be seen.
- Which release branch to backport (provide reason below if selected)
- Description for the changelog
Added "API" keyword before export lyd_check_mandatory_tree() function definition.
- A picture of a cute animal (not mandatory but encouraged)