data: expand the OP parsing with new API functions #19
Conversation
choppsv1
force-pushed
the
chopps/fix-parse_op
branch
from
aade806
to
76d1199
Compare
src/data.rs
Outdated
| data: impl AsRef<[u8]>, | ||
| format: DataFormat, | ||
| op_type: ffi::lyd_type::Type, | ||
| ) -> Result<DataNodeRef<'a, 'a>> { |
There was a problem hiding this comment.
I'm confused about this method signature.
lyd_parse_op() has the following params related to data trees:
* @param[in] parent Optional parent to connect the parsed nodes to.
* @param[out] tree Optional full parsed data tree. If @p parent is set, set to NULL.
* @param[out] op Optional pointer to the operation (action/RPC) node.
You're passing a NULL pointer to parent, which is the only input data tree (tree and op are output parameters). So why does this function need to take &mut self?
The why I'm seeing this, lyd_parse_op will allocate a new data tree to op, which is independent from &mut self, so the return value must be a DataTree that owns that data. Returning a DataNodeRef means op will leak once it goes out of scope.
There was a problem hiding this comment.
I believe you are correct. I actually am using the restconf routine in DataNodeRef and wrote these 4 functions for completeness sake to cover the other 3 transport based OP types. I'll redo these.
src/data.rs
Outdated
| @@ -530,6 +530,97 @@ impl<'a> DataTree<'a> { | |||
| Ok(unsafe { DataTree::from_raw(context, rnode) }) | |||
| } | |||
|
|
|||
| pub fn _parse_op( | |||
There was a problem hiding this comment.
This should probably be a private function.
That's possible. We have unit tests where we use The doc comments of The doc mentions |
choppsv1
force-pushed
the
chopps/fix-parse_op
branch
2 times, most recently
from
b266ea0
to
b283ba2
Compare
Welcome to Codecov 🎉Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests. Thanks for integrating Codecov - We've got you covered ☂️ |
choppsv1
force-pushed
the
chopps/fix-parse_op
branch
3 times, most recently
from
1b31963
to
f5abb0a
Compare
| @@ -28,6 +28,12 @@ pub struct DataTree<'a> { | |||
| raw: *mut ffi::lyd_node, | |||
| } | |||
|
|
|||
| #[derive(Debug)] | |||
| pub struct DataTreeOwningRef<'a> { | |||
There was a problem hiding this comment.
nit: please add a doc comment for this struct
src/data.rs
Outdated
| Ok(unsafe { DataTreeOwningRef::from_raw(tree, raw) }) | ||
| } | ||
|
|
||
| pub fn noderef(&'a self) -> DataNodeRef<'a, 'a> { |
src/data.rs
Outdated
| // Create input handler. | ||
| let mut ly_in = std::ptr::null_mut(); | ||
| let ret = if data.as_ref().is_empty() { | ||
| let blank = CString::new("").unwrap(); |
There was a problem hiding this comment.
Is this really necessary?
I just checked the source of ly_in_new_memory() and it does this:
LIBYANG_API_DEF LY_ERR
ly_in_new_memory(const char *str, struct ly_in **in)
{
LY_CHECK_ARG_RET(NULL, str, in, LY_EINVAL);I believe you want this function to do nothing when data is empty, instead of returning an error. Could you confirm?
There was a problem hiding this comment.
Correct; however, this code is buggy as it doesn't allocate new memory it just keeps a pointer to the passed in data. So I need to fix it.
choppsv1
force-pushed
the
chopps/fix-parse_op
branch
2 times, most recently
from
493add4
to
55a8dbf
Compare
Signed-off-by: Christian Hopps <[email protected]>
The new functions handle the 3 NETCONF and 3 RESTCONF variations. It also seems like the existing `fn parse_op` is broken as it returns the envelope node which will not be the actual tree, but opaque data. Signed-off-by: Christian Hopps <[email protected]>
Signed-off-by: Christian Hopps <[email protected]>
choppsv1
force-pushed
the
chopps/fix-parse_op
branch
from
55a8dbf
to
41858cc
Compare
The new functions handle the 3 NETCONF and 3 RESTCONF variations. It also seems like the existing
fn parse_opis broken as it returns the envelope node which will not be the actual tree, but opaque data.