sysrepo / sysrepo-cpp

C++ bindings for the sysrepo library
BSD 3-Clause "New" or "Revised" License
6 stars 5 forks source link

Wrap for sr_get_items and sr_get_subtree #13

Closed jjin62 closed 1 year ago

jjin62 commented 1 year ago

I understand that getData is a super set of all GET, but sr_get_items and sr_get_subtree are useful(more efficient in many use cases). I wonder if you have a plan to add these?

syyyr commented 1 year ago

Hi, there's no plan to support sr_get_items.

more efficient in many use cases

What use cases and what do you mean by "efficient"? I'm pretty sure sr_get_items just converts the result of sr_get_data.

michalvasko commented 1 year ago

No, those functions (especially sr_get_item()) really may be more efficient. All the getters first build the base data. While sr_get_data() then filters all the requested data, duplicates subtrees of the result, and finally merges them all into one tree, sr_get_item() only finds the single node and transforms it into a sr_val_t structure, so some node duplication is usually avoided and sr_val_t has fewer members than struct lyd_node *, anyway.

syyyr commented 1 year ago

Working with sr_val_t was never a pleasure, I don't understand the need for it. I suppose sr_get_item is somewhat more efficient, but it still needs to use libyang for filtering right? I'm not sure how these functions work internally. Maybe there needs to be a sr_get_a_single_node function? That way you could retrieve a single node and not have to deal with sr_val_t and still get some performance benefit.

michalvasko commented 1 year ago

Yes, I suppose the option to duplicate only a single node would be as efficient as sr_get_item(). I will add it to sysrepo in the next few days.

jjin62 commented 1 year ago

thanks for the comments. My "efficiency" also means easy write application using sysrepo APIs. In this case, we can get all the leaf nodes using get_items in old sysrepo cpp. I think various get APIs in sysrepo are convenient, as we don't need to know how libyang works.

jktjkt commented 1 year ago

Yes, I suppose the option to duplicate only a single node would be as efficient as sr_get_item(). I will add it to sysrepo in the next few days.

thanks; we'll add C++bindings for these once that patch lands

I think various get APIs in sysrepo are convenient, as we don't need to know how libyang works

There's also value in the simplicity of these APIs. Once the patch mentioned above lands, you've got to use this one:

session.getItem("/example:foor/bar")->asTerm().value().valueStr();

What do you want to happen when you get an item that doesn't exist? An exception? A std::nullopt? What happens when that data exists, but it is not a leaf or a leaf-list item? Unfortunately, there's a policy decision to be made after each -> or even .. Since it's super-easy to write a trivial wrapper, we decided to skip that and to only work with libyang's data structures. We will certainly not be wrapping sr_val_t, ever. Perhaps we might add a wrapper for getting a single item as a string, but there will be no "complementary" methods for fetching ints, etc.

jjin62 commented 1 year ago

Here are some thoughts:

thanks.

syyyr commented 1 year ago

getItem() returns std::nullopt for non-exit node, as same as getData(); For path to a non-Term node, you can return a DataNode with path and no-value, like C API get_item();

Those seem feasible. We just need to wrap the new sr_get_node function.

we are using session.get_items() to get all the leaf nodes under a path, but it is not available now. So what is the best way to get all leaf nodes of a subtree/DataNode?

IMHO, just something like:


for (const auto& node : session.getData(...).childrenDfs()) {
    if (node.schema().nodeType() == libyang::NodeType::Leaf) {
        // ...
    }
}
jktjkt commented 1 year ago

@jjin62 the patch is ready, feedback is welcome.

@michalvasko: I wasn't able to get that function to set the output node to NULL while not returning an error. Is that possible, and what kind of XPath might end up doing that? I tried a valid XPath where the leaf was not set, but that returns SR_ERR_NOT_FOUND.

michalvasko commented 1 year ago

It does return SR_ERR_NOT_FOUND but also sets the output to NULL. It is explicitly mentioned so that users know they can test either.