Re: [PATCH net-next v3 03/24] ovpn: add basic netlink support

Antonio Quartulli
2024-05-08T07:41:35Z

On 08/05/2024 02:10, Jakub Kicinski wrote:
> On Mon, 6 May 2024 03:16:16 +0200 Antonio Quartulli wrote:
>> + name: nonce_tail_size
>
> nit: typically we hyphenate the names in YAML and C codegen replaces
> the hyphens with underscores (and converts to uppercase)

ACK will go with hyphens.

>
>> + exact-len: OVPN_NONCE_TAIL_SIZE
>
> speaking of which - is the codegen buggy or this can be nonce_tail_size?
> (or rather nonce-tail-size)

yeah, something was wrong.
I used 'nonce_tail_size' at first, but it did not get converted,
therefore I hardcoded the final define name.

From what you say this seems to be unexpected.
Will check what the script does.

>
>> + -
>> + name: pad
>> + type: pad
>
> You shouldn't need this, now that we have uint.
> replace nla_put_u64_64bit() with nla_put_uint().

ACK

> Unfortunately libnl hasn't caught up so you may need to open code
> the getter a little in user space CLI.

ok, no big deal.

>
> BTW I'd also bump the packet counters to uint.
> Doesn't cost much if they don't grow > 32b and you never know..

Ok, will do

>
>> + request:
>> + attributes:
>> + - ifname
>> + - mode
>> + reply:
>> + attributes:
>> + - ifname
>
> The attribute lists
>
>> + struct net_device *dev;
>> + int ifindex;
>> +
>> + if (!attrs[OVPN_A_IFINDEX])
>
> GENL_REQ_ATTR_CHECK()

ACK, I must have missed this one.

>
>> + return ERR_PTR(-EINVAL);
>> +
>> + ifindex = nla_get_u32(attrs[OVPN_A_IFINDEX]);
>> +
>> + dev = dev_get_by_index(net, ifindex);
>> + if (!dev)
>> + return ERR_PTR(-ENODEV);
>> +
>> + if (!ovpn_dev_is_valid(dev))
>> + goto err_put_dev;
>> +
>> + return dev;
>> +
>> +err_put_dev:
>> + dev_put(dev);
>
> NL_SET_BAD_ATTR(info->extack, ...[OVPN_A_IFINDEX])

Oh, thanks for pointing this out.

>
>> + return ERR_PTR(-EINVAL);

--
Antonio Quartulli
OpenVPN Inc.