On Thu, 2019-05-02 at 07:36 -0600, David Ahern wrote: > On 5/2/19 7:32 AM, Michal Kubecek wrote: > > Wouldn't it mean effecitvely ending up with only one command (in > > genetlink sense) and having to distinguish actual commands with > > atributes? Even if I wanted to have just "get" and "set" command, common > > policy wouldn't allow me to say which attributes are allowed for each of > > them. > > yes, I have been stuck on that as well. > > There are a number of RTA attributes that are only valid for GET > requests or only used in the response or only valid in NEW requests. > Right now there is no discriminator when validating policies and the > patch set to expose the policies to userspace
Yeah. As I've been discussing with Pablo in various threads recently, this is definitely something we're missing. As I said there though, I think it's something we should treat as orthogonal to the policies. I haven't looked at your ethtool patches really now (if you have a git tree that'd be nice), but I saw e.g. +When appropriate, network device is identified by a nested attribute named +ETHA_*_DEV. This attribute can contain + + ETHA_DEV_INDEX (u32) device ifindex + ETHA_DEV_NAME (string) device name Presumably, this is valid for each and every command, right? I'm not sure I understand the "ETHA_*_DEV" part, but splitting the policy per command means that things like this that are available/valid for each command need to be stated over and over again. This opens up the very easy possibility that you have one command that takes an ETHA_DEV_INDEX as u32, and another that - for some reason - takes a u64 for example, or similar confusion between the same attribute stated in different policies. This is why I believe that when we have a flat namespace for attributes, like ETHA_*, we should also have a flat policy for those attributes, and that's why I made the genetlink to have a single policy. At the same time, I do realize that this is not ideal. So far I've sort of pushed this to be something that we should treat orthogonally to the validation for the above reasons, i.e. *not* state this specifically in the policy. If we were able to express this in C, I'd probably say we should have something like static const struct genl_ops ops[] = { { .cmd = MY_CMD, .doit = my_cmd_doit, .valid_attrs = { MY_ATTR_A, MY_ATTR_B }, }, ... }; However, there's no way to express this in C code, except for static const u16 my_cmd_valid_attrs[] = { MY_ATTR_A, MY_ATTR_B, 0 }; static const struct genl_ops ops[] = { { .cmd = MY_CMD, .doit = my_cmd_doit, .valid_attrs = my_cmd_valid_attrs, }, ... }; which is clearly ugly to write. We could generate this code from a domain-specific language like Pablo suggested, but I'm not really sure that's ideal either. Regardless, I think we should solve this problem orthogonally from the policy, given that otherwise we can end up with the same attribute meaning different things in different commands. johannes