[Intel-wired-lan] [PATCH v5 net-next 02/12] net-shapers: implement NL get operation
Jakub Kicinski
kuba at kernel.org
Fri Aug 30 18:39:00 UTC 2024
On Fri, 30 Aug 2024 12:55:05 +0200 Paolo Abeni wrote:
> On 8/30/24 03:20, Jakub Kicinski wrote:>> +/* Initialize the context
> fetching the relevant device and
> >> + * acquiring a reference to it.
> >> + */
> >> +static int net_shaper_ctx_init(const struct genl_info *info, int type,
> >> + struct net_shaper_nl_ctx *ctx)
> >> +{
> >> + struct net *ns = genl_info_net(info);
> >> + struct net_device *dev;
> >> + int ifindex;
> >> +
> >> + memset(ctx, 0, sizeof(*ctx));
> >> + if (GENL_REQ_ATTR_CHECK(info, type))
> >> + return -EINVAL;
> >> +
> >> + ifindex = nla_get_u32(info->attrs[type]);
> >
> > Let's limit the 'binding' thing to just driver call sites, we can
> > redo the rest easily later. This line and next pretends to take
> > "arbitrary" type but clearly wants a ifindex/netdev, right?
>
> There is a misunderstanding. This helper will be used in a following
> patch (7/12) with a different 'type' argument:
> NET_SHAPER_A_BINDING_IFINDEX. I've put a note in the commit message, but
> was unintentionally dropped in one of the recent refactors. I'll add
> that note back.
What I'm saying is that if you want to prep the ground for more
"binding" types you should also add:
if (type != ...IFINDEX) {
/* other binding types are TBD */
return -EINVAL;
}
> I hope you are ok with the struct net_shaper_binding * argument to most
> helpers? does not add complexity, will help to support devlink objects
> and swapping back and forth from/to struct net_device* can't be automated.
I am "okay" in the American sense of the word which AFAIU is "unhappy
but won't complain unless asked".
> > Maybe send a patch like this, to avoid having to allocate this space,
> > and special casing dump vs doit:
> >
> > diff --git a/include/net/genetlink.h b/include/net/genetlink.h
> > index 9ab49bfeae78..7658f0885178 100644
> > --- a/include/net/genetlink.h
> > +++ b/include/net/genetlink.h
> > @@ -124,7 +124,8 @@ struct genl_family {
> > * @genlhdr: generic netlink message header
> > * @attrs: netlink attributes
> > * @_net: network namespace
> > - * @user_ptr: user pointers
> > + * @ctx: storage space for the use by the family
> > + * @user_ptr: user pointers (deprecated, use ctx instead)
> > * @extack: extended ACK report struct
> > */
> > struct genl_info {
> > @@ -135,7 +136,10 @@ struct genl_info {
> > struct genlmsghdr * genlhdr;
> > struct nlattr ** attrs;
> > possible_net_t _net;
> > - void * user_ptr[2];
> > + union {
> > + u8 ctx[48];
> > + void * user_ptr[2];
> > + };
> > struct netlink_ext_ack *extack;
> > };
>
> Makes sense. Plus likely:
>
> #define NETLINK_CTX_SIZE 48
>
> and use such define above and in linux/netlink.h
Aha, would be good to also have a checking macro. Maybe rename
NL_ASSERT_DUMP_CTX_FITS()
to apply more broadly? or add a new one? Weak preference for former.
More information about the Intel-wired-lan
mailing list