Include NLA header on mcast groups and ops. #8
Conversation
This is a nested list of NLAs, so we need to include a header on each entry to properly parse. The new test verifies that emit and parse now both expect the same mcast group format.
Codecov Report
@@ Coverage Diff @@
## main #8 +/- ##
===========================================
+ Coverage 34.20% 56.40% +22.19%
===========================================
Files 11 11
Lines 573 734 +161
===========================================
+ Hits 196 414 +218
+ Misses 377 320 -57
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
There was a problem hiding this comment.
This is not elegant and prone to errors when there is padding.
How about:
GenlCtrlAttrs::McastGroups(Vec<McastGroup>)struct McastGroup { nlas: Vec<McastGrpAttrs>}- impl Nla for
McastGroup
With this, we can have nlas.as_slice().buffer_len() and nlas.as_slice().emit(buffer) saving us from worry about headers.
Meanwhile, please fix CTRL_ATTR_OPS also.
Updates vec-based ctrl NLAs to use an inner NLA with its own header.
|
Sure, pushed an updated version and fixed ops too. |
src/ctrl/nlas/mcast.rs
Outdated
|
|
||
| impl Nla for McastGroup { | ||
| fn value_len(&self) -> usize { | ||
| self.nlas.iter().map(|nla| nla.buffer_len()).sum() |
There was a problem hiding this comment.
Please use self.nlas.as_slice().buffer_len()
src/ctrl/nlas/mcast.rs
Outdated
| } | ||
|
|
||
| fn kind(&self) -> u16 { | ||
| NLA_F_NESTED |
There was a problem hiding this comment.
The kind() is not NLA_F_NESTED. It is index + 1. The kernel code is:
nla_grps = nla_nest_start_noflag(skb, CTRL_ATTR_MCAST_GROUPS);
if (nla_grps == NULL)
goto nla_put_failure;
for (i = 0; i < family->n_mcgrps; i++) {
struct nlattr *nest;
const struct genl_multicast_group *grp;
grp = &family->mcgrps[i];
nest = nla_nest_start_noflag(skb, i + 1); // <<<<<< HERE
if (nest == NULL)
goto nla_put_failure;Please check BondIpAddrNlaList in netlink-packet-route as an example.
There was a problem hiding this comment.
Ah, thanks for the reference. Updated based on your example.
src/ctrl/nlas/ops.rs
Outdated
| } | ||
|
|
||
| fn kind(&self) -> u16 { | ||
| NLA_F_NESTED |
There was a problem hiding this comment.
This should be index of Op. This is kernel code:
nla_ops = nla_nest_start_noflag(skb, CTRL_ATTR_OPS);
if (nla_ops == NULL)
goto nla_put_failure;
while (genl_op_iter_next(&i)) {
struct nlattr *nest;
u32 op_flags;
op_flags = i.flags;
if (i.doit.policy || i.dumpit.policy)
op_flags |= GENL_CMD_CAP_HASPOL;
nest = nla_nest_start_noflag(skb, genl_op_iter_idx(&i));
|
I've addressed your comments and updated the test cases to include proper indices and padding based on strace output. |
dylanswiggett
changed the title
This change is in a pull request upstream: rust-netlink/netlink-packet-generic#8 Bug: 128857 Test: New unit test. Change-Id: Id3ad21a119a2a77e6c272b55957e0c4df5a32c1d Reviewed-on: https://fuchsia-review.googlesource.com/c/fuchsia/+/870539 Commit-Queue: Dylan Swiggett <swiggett@google.com> Reviewed-by: Jeff Martin <martinjeffrey@google.com>
The emit logic for McastGroups doesn't include the required NLA header on nested NLAs. Without this change, netlink-packet-generic emits incorrect buffers if an mcast group is included, and cannot parse the bytes it writes.