rtnetlink: add a way to set netlink flags in requests#246
rtnetlink: add a way to set netlink flags in requests#246little-dude wants to merge 2 commits intomasterfrom
Conversation
There has been several PRs to add helper methods to set the flags in the netlink header, so it seems that this is something we should provide systematically. This PR introduces three new types to manipulate netlink flags: - `NewFlags`: flags used for "set" and "add" requests - `DelFlags`: flags used to "del" requests - `GetFlags:` flags used for "get" requests Each request now has a `set_flags` method to force a certain set of flags. This may interfere with other builder methods for certain requests, or may simply render the request ineffective, so the documentation always warns users.
|
@cathay4t @ffmancera I'd appreciate a review on this one if you have time, just to make sure I haven't broken anything. |
|
Will the new commit differ from the default flags of the original code? Does this mean that a major version number needs to be upgraded? And there are interfaces removed. |
|
New version (0.x+1) will be released as we changed(removed) an member of BTW, I am still reviewing the code. Personally I am reluctant on adding new dependency, need to weight the benefit and risk. |
cathay4t
left a comment
cathay4t
left a comment
There was a problem hiding this comment.
- The flags is for netlink, not rtnetlink.
- Differentiating AddFlag and DelFlag, add extra learning to user who already know kernel netlink. And also make the API complex.
- Human typed integer is hard to maintain and review comparing to kernel header file is using hex format.
- Exposing a public struct generated by a wrapper macros is dangerous, the maintainer bitflags might hold different understanding on backwards compatibility than this project.
- Allowing user to
set_flagvia a u16 integer is simple enough to solve this problem. Why make it complex using struct and rust macros? As you already say in comment, user who callset_flagsshould know what they are doing, those user should understand the integer use forset_flagsis bit flags, for them an u16 integer is simpler than a struct.
| /// Must be set on all request messages (typically from user | ||
| /// space to kernel space) | ||
| /// | ||
| /// **This flag is set by default** |
There was a problem hiding this comment.
No need to mention that as people can tell that via Default trait.
At the first time I noticed this line, I am wondering does this mean bitflags has magic on setting this constant as default?
| @@ -0,0 +1,132 @@ | |||
| bitflags::bitflags! { | |||
| pub struct GetFlags: u16 { | |||
There was a problem hiding this comment.
The linux/netlink.h does not say these flags is for rtnetlink only. I think there is no need to distinguish Get and Del, in stead, just name them as NetlinkMessageFlag and place to netlink-proto.
The user of this crate should aware only small potion of those flags is available for get , delete and etc.
| /// `CAP_NET_ADMIN` capability or a effective UID of 0. | ||
| const ATOMIC = 1024; | ||
| /// Equivalent to `ROOT|MATCH`. | ||
| const DUMP = 768; |
There was a problem hiding this comment.
Using integer is vulnerable to human error.
Please just copy kernel code and use sed or other regex to convert them into rust code.
The kernel code is using this kind format:
#define NLM_F_ATOMIC 0x400
#define NLM_F_DUMP (NLM_F_ROOT|NLM_F_MATCH)So, the rust code should be
const ATOMIC = 0x400;
const DUMP = Self.ROOT | Self.MATCH;
3 participants
There has been several PRs to add helper methods to set the flags in
the netlink header, so it seems that this is something we should
provide systematically.
This PR introduces three new types to manipulate netlink flags:
NewFlags: flags used for "set" and "add" requestsDelFlags: flags used to "del" requestsGetFlags:flags used for "get" requestsEach request now has a
set_flagsmethod to force a certain set offlags. This may interfere with other builder methods for certain
requests, or may simply render the request ineffective, so the
documentation always warns users.