[nexthop] Add a bunch of setters/getters to for fields in struct rtnl_nh #435
Conversation
libnl-route-3.sym
Outdated
| rtnl_link_is_bond; | ||
| rtnl_nh_get_family; | ||
| rtnl_nh_get_oif; | ||
| rtnl_nh_set_group; |
lib/route/nh.c
Outdated
| return nexthop->nh_family; | ||
| } | ||
|
|
||
| int rtnl_nh_set_group(struct rtnl_nh *nexthop, nl_nh_group_info_t *entries, |
There was a problem hiding this comment.
I think this should be a const nl_nh_group_info_t * pointer.
| return -NLE_INVAL; | ||
|
|
||
| if (size == 0) | ||
| return -NLE_INVAL; |
There was a problem hiding this comment.
is there a problem with allowing to set this to zero?
| dst->nh_family = src->nh_family; | ||
| dst->nh_id = src->nh_id; | ||
| dst->nh_oif = src->nh_oif; | ||
| dst->nh_group_type = src->nh_group_type; |
| if (!(nexthop->ce_mask & NH_ATTR_GROUP_TYPE)) | ||
| return -NLE_INVAL; | ||
|
|
||
| return (int)nexthop->nh_group_type; |
There was a problem hiding this comment.
in this case, we encode the error value and the success value in int. I think that is fine for NHA_GROUP_TYPE.
At least, as long as we are sure that this value is always just an u16 (that fits inside the positive range of int).
In that case, nh_group_type in struct rtnl_nh is wrongly defined as uint32_t. Could you please fix that too?
Or do you think there might be cases where it's not u16? In that case, the getter would have to return the value and the error code separately.
include/netlink/route/nh.h
Outdated
|
|
||
| extern int rtnl_nh_set_res_group_unbalanced_timer(struct rtnl_nh *, | ||
| uint32_t unbalanced_timer); | ||
| extern int rtnl_nh_get_res_group_unbalanced_timer(struct rtnl_nh *); |
There was a problem hiding this comment.
for rtnl_nh_get_res_group_idle_timer() and rtnl_nh_get_res_group_unbalanced_timer() it seems that the full range of values is uint32_t.
We cannot encode that in an int return value. The getters need to have a separate uint32_t *out_value parameter.
lib/route/nh.c
Outdated
| } | ||
| if (rg[NHA_RES_GROUP_UNBALANCED_TIMER]) { | ||
| nexthop->res_grp_unbalanced_timer = nla_get_u32( | ||
| rg[NHA_RES_GROUP_UNBALANCED_TIMER]); |
There was a problem hiding this comment.
you should pass a policy argument to nla_parse_nested().
Otherwise, you cannot blindly trust that the provided value is in fact a nla_get_u32().
Theoretically, you could also check each attribute individually. But seems simpler to create a policy.
Add necessary setters to allow setting these fields on a nexthop in preparation of pushing these nexthops down to the kernel through netlink. Signed-off-by: Christoph Paasch <[email protected]>
This will be needed to be able to send correct netlink messages. Signed-off-by: Christoph Paasch <[email protected]>
Allow to add a group to a nexthop. Signed-off-by: Christoph Paasch <[email protected]>
Necessary for specifying resilient groups. Signed-off-by: Christoph Paasch <[email protected]>
The resilient nexthop group defines a set of attributes: bucket-size, idle-timer and unbalanced-timer. Add necessary getters/setters for these. Signed-off-by: Christoph Paasch <[email protected]>
cpaasch-oai
force-pushed
the
t/nh_set_id
branch
from
4d0b4ed to
b658029
Compare
|
Actually - as I am building a lot of tests for rtnl_nh I realize that things are inconsistent across the board regarding input-checks (beyond the code I'm adding here). Sometimes there is : And in other cases there is no null-check at all. Do you think we should unify this ? IMO I would say that no null-check is better to avoid silent failures. But well - it's a judgement call :) |
|
merged, thank you! |
2 participants
This pull-request further works towards an implementation where libnl can push nexthop configuration similar to
ip nexthopdown to the kernel.We need a bunch of setters to complete this.
The
rtnl_nh_addto actually create the netlink message comes in a follow-up PR.