Re: [PATCH libmnl] src: attr: Add mnl_attr_get_uint() function

[Petr Machata <petrm@nvidia.com>]

Pablo Neira Ayuso <pablo@netfilter.org> writes:

> On Mon, Sep 30, 2024 at 07:01:42PM +0200, Pablo Neira Ayuso wrote:
>> On Mon, Sep 30, 2024 at 06:25:17PM +0200, Petr Machata wrote:
>> > 
>> > Pablo Neira Ayuso <pablo@netfilter.org> writes:
>> > 
>> > > 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?
>> > 
>> > This follows the iproute2 helper, where I was asked to support >32-bit
>> > fields purely as a service to the users, so that one helper can be used
>> > for any integral field.
>> 
>> Which helper are your referring to? Is it modeled after NLA_UINT?
>> 
>> I don't think this patch is fine. This also returns -1ULL so there is
>> no way to know if size is not correct or payload length is 64 bits
>> using UINT64_MAX?

That's no different from the other mnl_attr_get_uX() helpers that just
assume the user has passed in the right attribute type.

> I found it:
>
> static inline __u64 rta_getattr_uint(const struct rtattr *rta)

That's the one.

> This only has one user in the tree so far, right?

Well, two, but yeah, the u8 / u16 stuff is not currently used.

> include/libnetlink.h:static inline __u64 rta_getattr_uint(const struct rtattr *rta)
> ip/ipnexthop.c:                 nh_grp_stats->packets = rta_getattr_uint(rta);
> ip/ipnexthop.c:                 nh_grp_stats->packets_hw = rta_getattr_uint(rta);
>
> is this attribute for ipnexthop of NLA_UINT type?

Yeah, both NHA_GROUP_STATS_ENTRY_PACKETS and
NHA_GROUP_STATS_ENTRY_PACKETS_HW are NLA_UINT.