Messages in this thread | | | Date | Tue, 17 May 2022 23:37:13 -0400 | Subject | Re: [RFC net-next] bonding: netlink error message support for options | From | Jonathan Toppins <> |
| |
On 5/17/22 19:54, Jakub Kicinski wrote: > On Tue, 17 May 2022 15:44:19 -0700 Stephen Hemminger wrote: >> On Tue, 17 May 2022 16:31:19 -0400 >> Jonathan Toppins <jtoppins@redhat.com> wrote: >> >>> This is an RFC because the current NL_SET_ERR_MSG() macros do not support >>> printf like semantics so I rolled my own buffer setting in __bond_opt_set(). >>> The issue is I could not quite figure out the life-cycle of the buffer, if >>> rtnl lock is held until after the text buffer is copied into the packet >>> then we are ok, otherwise, some other type of buffer management scheme will >>> be needed as this could result in corrupted error messages when modifying >>> multiple bonds. >> >> Might be better for others in long term if NL_SET_ERR_MSG() had printf like >> semantics. Surely this isn't going to be first or last case. >> >> Then internally, it could print right to the netlink message. > > Dunno. I think pointing at the bad attr + exposing per-attr netlink > parsing policy + a string for a human worked pretty well so far. > IMHO printf() is just a knee jerk reaction, especially when converting > from netdev_err().
For some subsystems it is not a convert from netdev_err, it is an AND. In this RFC there are instances where changing the message from netdev_err() to the macro was trivial;
@@ -240,12 +243,14 @@ static int bond_changelink(struct net_device *bond_dev, st ruct nlattr *tb[], int arp_interval = nla_get_u32(data[IFLA_BOND_ARP_INTERVAL]);
if (arp_interval && miimon) { - netdev_err(bond->dev, "ARP monitoring cannot be used with MII monitoring\n"); + NL_SET_ERR_MSG(extack, + "ARP monitoring cannot be used with MII monitoring"); return -EINVAL; }
These are trivial because the path does not have to care about sysfs or some other legacy configuration interface. These macros become rather annoying to use once a system needs to support multiple configuration paths and is trying to utilize as much common configuration code[0] as possible so that all interfaces largely operate the same way.
> > Augmenting structured information is much, much better long term. > > To me the never ending stream of efforts to improve printk() is a > proof that once we let people printf() at will, efforts to contain > it will be futile. > At least for bonding I was trying to reuse the most amount of code which needs to deal with both sysfs and netlink. And I don't think it is a good idea to split the code paths, so if I am suppose to use statically allocated strings to support netlink errors that basically means anything that has to support multiple interfaces gets to sprinkle `if (extack)` everywhere[0]. Not great. The ownership model of the error buffer seems odd to me with the current macros, I am suppose to set a pointer in a structure subsystem X didn't allocate and has no control over its lifetime. Then netlink takes this pointer and does whatever with it. And somehow subsystem X is suppose to guarantee the pointer's lifetime exists forever, making a `const static char[]` buffer the only option. I don't understand why netlink doesn't provide the buffer and a subsystem just populates it. Using memcpy or snprintf doesn't matter, to me its a lifetime issue that makes the API not great to work with when you have to handle cases other than netlink.
Also as Joe Perches points out in this thread[1,2] the way the macros are written it is bloating the kernel because the error messages are getting duplicated for subsystems that need to support multiple configuration interfaces.
-Jon
[0] https://lore.kernel.org/netdev/e6b78ce8f5904a5411a809cf4205d745f8af98cb.1628650079.git.jtoppins@redhat.com/ [1] https://lore.kernel.org/netdev/cover.1628306392.git.jtoppins@redhat.com/ [2] https://lore.kernel.org/netdev/c8b69905c995ab887633ef11862705ee66c60aad.camel@perches.com/
| |