Pablo Neira Ayuso <pablo@xxxxxxxxxxxxx> writes:
> On Mon, Sep 30, 2024 at 07:10:27PM +0200, Pablo Neira Ayuso wrote:
>> 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@xxxxxxxxxxxxx> 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?
>>
>> I found it:
>>
>> static inline __u64 rta_getattr_uint(const struct rtattr *rta)
>>
>> This only has one user in the tree so far, right?
>
> Well, this is a matter of documenting behaviour.
>
>> 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?
>
> But it seems intention is to support NLA_UINT according to iproute's
> commit.
>
> commit 95836fbf35d352f7c031ddac2e6093a935308cc9
> Author: Petr Machata <petrm@xxxxxxxxxx>
> Date: Thu Mar 14 15:52:12 2024 +0100
>
> libnetlink: Add rta_getattr_uint()
>
> NLA_UINT attributes have a 4-byte payload if possible, and an 8-byte one if
> necessary. Add a function to extract these. Since we need to dispatch on
> length anyway, make the getter truly universal by supporting also u8 and
> u16.
>
> so it went further to make it universal for 2^3..2^6 values.
>
> I am going to submit a patch to provide more info on this helper function.
Yeah, sorry, I wrote this to explain the rationale:
> 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.
... but of course I should have written <32-bit payloads, not >32-bit.
