> From: Pablo Neira Ayuso <pablo@xxxxxxxxxxxxx>
> Sent: Monday, 30 September 2024 15:48
> To: Jakub Kicinski <kuba@xxxxxxxxxx>
> Cc: Danielle Ratson <danieller@xxxxxxxxxx>; Phil Sutter <phil@xxxxxx>;
> netfilter-devel@xxxxxxxxxxxxxxx; fw@xxxxxxxxx; mlxsw <mlxsw@xxxxxxxxxx>
> Subject: Re: [PATCH libmnl] src: attr: Add mnl_attr_get_uint() function
>
> On Mon, Sep 30, 2024 at 01:45:09PM +0200, Jakub Kicinski wrote:
> > On Mon, 30 Sep 2024 12:56:20 +0200 Pablo Neira Ayuso wrote:
> > > On Mon, Sep 30, 2024 at 12:28:08PM +0200, Pablo Neira Ayuso wrote:
> > > > On Sun, Sep 29, 2024 at 10:42:44AM +0000, Danielle Ratson wrote:
> > > > > Hi,
> > > > >
> > > > > Is there a plan to build a new version soon?
> > > > > I am asking since I am planning to use this function in ethtool.
> > > >
> > > > ASAP
> > >
> > > but one question before... Is this related to NLA_UINT in the kernel?
> > >
> > > /**
> > > * nla_put_uint - Add a variable-size unsigned int to a socket
> > > buffer
> > > * @skb: socket buffer to add attribute to
> > > * @attrtype: attribute type
> > > * @value: numeric value
> > > */
> > > static inline int nla_put_uint(struct sk_buff *skb, int attrtype,
> > > u64 value) {
> > > u64 tmp64 = value;
> > > u32 tmp32 = value;
> > >
> > > if (tmp64 == tmp32)
> > > return nla_put_u32(skb, attrtype, tmp32);
> > > return nla_put(skb, attrtype, sizeof(u64), &tmp64); }
> > >
> > > if I'm correct, it seems kernel always uses either u32 or u64.
> > >
> > > Userspace assumes u8 and u16 are possible though:
> > >
> > > +/**
> > > + * mnl_attr_get_uint - returns 64-bit unsigned integer attribute.
> > > + * \param attr pointer to netlink attribute
> > > + *
> > > + * This function returns the 64-bit value of the attribute payload.
> > > + */
> > > +EXPORT_SYMBOL uint64_t mnl_attr_get_uint(const struct nlattr *attr)
> > > +{
> > > + switch (mnl_attr_get_payload_len(attr)) {
> > > + case sizeof(uint8_t):
> > > + return mnl_attr_get_u8(attr);
> > > + case sizeof(uint16_t):
> > > + return mnl_attr_get_u16(attr);
> > > + case sizeof(uint32_t):
> > > + return mnl_attr_get_u32(attr);
> > > + case sizeof(uint64_t):
> > > + return mnl_attr_get_u64(attr);
> > > + }
> > > +
> > > + return -1ULL;
> > > +}
> > >
> > > Or this is an attempt to provide a helper that allows you fetch for
> > > payload value of 2^3..2^6 bytes?
> >
> > No preference here, FWIW. Looks like this patch does a different thing
> > than the kernel. But maybe a broader "automatic" helper is useful for
> > user space code.
>
> Not sure. @Danielle: could you clarify your intention?
Hi,
Thanks for all the comments.
As I see it, there are at least two occurrences in ethtool for a code that tries to overcome the lack of this kind of uint helper when the uint attribute type exists in the kernel and seems like more recommended to use then before.
Therefore, as I see it, this helper seems more reasonable to have.
But I am trying to understand, are we discussing the need of the patch at all? Or some nits about the way it is implemented? Or something else?
I am asking since the patch was already taken by @Pablo Neira Ayuso.
>
> If this is to support NLA_UINT, I'd prefer to stick to NLA_UINT semantics.
>
> @Jakub: is there any plan to augment NLA_UINT in the future? What the
> assumption from userspace that this will always return 32-bits else 64-bits
> value?
>
> Thanks.
