mgmtd get-data request expansion #15154
Conversation
idryzhov
commented
Jan 13, 2024
idryzhov
commented
- add the ability to request state-only, config-only, or all data in the same request
- add the ability to request the exact node instead of the whole data tree
- a couple of fixes for existing yang-related functions
- tests for the new functionality
idryzhov
force-pushed
the
mgmt-get-data
branch
from
67d4d3c to
c5f09aa
Compare
lib/mgmt_msg_native.h
Outdated
| enum mgmt_msg_get_data_content { | ||
| GET_CONTENT_STATE, | ||
| GET_CONTENT_CONFIG, | ||
| GET_CONTENT_ALL, |
There was a problem hiding this comment.
I think this is a bit more natural as flags, so GET_DATA_FLAG_STATE = 0x1, GET_DATA_FLAG_CONFIG = 0x2. This allows for CHECK_FLAGS(content, GET_DATA_FLAG_STATE) for both the STATE and the ALL case, for example.
There was a problem hiding this comment.
Done. I changed the "exact" field to a flag as well.
mgmtd/mgmt_txn.c
Outdated
| @@ -2385,6 +2386,55 @@ int mgmt_txn_send_get_tree_oper(uint64_t txn_id, uint64_t req_id, | |||
| get_tree->simple_xpath = simple_xpath; | |||
| get_tree->xpath = XSTRDUP(MTYPE_MGMTD_XPATH, xpath); | |||
|
|
|||
| if (content != GET_CONTENT_STATE) { | |||
There was a problem hiding this comment.
So with flags this becomes CHECK_FLAG(content, GET_DATA_FLAG_CONFIG) which is much clearer IMO.
| uint8_t resv2[7]; | ||
|
|
||
| alignas(8) char xpath[]; | ||
| }; |
There was a problem hiding this comment.
Why not just replace mgmt_msg_get_tree with this? Even with the expanded functionality coming in later commits? I don't see any point in having a specialized get-tree message that can be exactly replaced by this one with a couple extra params.
There was a problem hiding this comment.
I think the code is more clear if we have separate messages for the frontend and for the backend. There are new fields in the frontend message and there will be more later (like datastore, with-defaults, etc., to cover the whole NETCONF/RESTCONF functionality). And all these new fields don't make sense on the backend.
We should traverse all top-level siblings, not only the first one. Signed-off-by: Igor Ryzhov <[email protected]>
idryzhov
force-pushed
the
mgmt-get-data
branch
from
c5f09aa to
539c623
Compare
lib/mgmt_msg_native.h
Outdated
| /* Flags for get-data request */ | ||
| #define GET_DATA_FLAG_STATE 0x01 /* get only "config false" data */ | ||
| #define GET_DATA_FLAG_CONFIG 0x02 /* get only "config true" data */ | ||
| #define GET_DATA_FLAG_ALL (GET_DATA_FLAG_STATE | GET_DATA_FLAG_CONFIG) |
There was a problem hiding this comment.
I almost put in my earlier comment, ditch ALL, should have... Please get rid of the ALL #define. It's just more clear what's going on in the code when one or both flags are present when need be. You are adding EXACT as a flag which is really incompatible with ALL. :)
mgmtd/mgmt_vty.c
Outdated
| flags = GET_DATA_FLAG_CONFIG; | ||
| } else { | ||
| flags = GET_DATA_FLAG_STATE; | ||
| } |
There was a problem hiding this comment.
Suggest:
uint8_t flags = cont ? GET_DATA_FLAG_CONFIG : GET_DATA_FLAG_STATE;
if (cont && cont[0] == 'w')
flags |= GET_DATA_FLAG_STATE;
There was a problem hiding this comment.
Thanks, I used your code.
mgmtd has to know if netns is used as the vrf backend to correctly process interface names in northbound. Signed-off-by: Igor Ryzhov <[email protected]>
Signed-off-by: Igor Ryzhov <[email protected]>
Currently it's the same as get-tree request for the backend, but it is going to be expanded in the following commits. Signed-off-by: Igor Ryzhov <[email protected]>
idryzhov
force-pushed
the
mgmt-get-data
branch
2 times, most recently
from
7f2cd79 to
ddbb292
Compare
|
CI:rerun Retest for new ASAN Memory Leaks |
| LY_ERR err; | ||
|
|
||
| err = lyd_find_xpath(config->dnode, xpath, &set); | ||
| if (err) { |
There was a problem hiding this comment.
set is leaked (never ly_set_free(set, NULL)d)
Like in RESTCONF GET request and NETCONF get-data request, make it possible to request state-only, config-only, or all data. Signed-off-by: Igor Ryzhov <[email protected]>
Signed-off-by: Igor Ryzhov <[email protected]>
When creating an initial tree trunk for oper data walk, if the xpath
represents a leaf, the leaf is created with an incorrect empty value.
If it doesn't actually exist in daemon's oper data, its value is not
overwritten later and an empty value is returned in the result.
For example, when requesting
`/frr-interface:lib/interface[name='eth0']/description`, the result is:
```
{
"frr-interface:lib": {
"interface": [
{
"name": "eth0",
"description": ""
}
]
}
}
```
instead of an empty JSON that it should be.
Signed-off-by: Igor Ryzhov <[email protected]>
RESTCONF expects to receive the exact node as a result, not the whole data tree. Signed-off-by: Igor Ryzhov <[email protected]>
Signed-off-by: Igor Ryzhov <[email protected]>
idryzhov
force-pushed
the
mgmt-get-data
branch
from
ddbb292 to
2764344
Compare
