Re: [PATCH v3 mptcp-next 2/6] mptcp: introduce MPTCP_FULL_INFO getsockopt

[Florian Westphal <fw@strlen.de>]

Matthieu Baerts <matthieu.baerts@tessares.net> wrote:
> On 23/05/2023 20:25, Florian Westphal wrote:
> > 	__u32		size_subflow_full_info;         /* size of this structure in userspace */
> 
> Why do we need this field? Can we not use the option length (int __user
> *optlen) provided by the userspace?

Right, yes, we can (and should) use __user *optlen instead
of this explicit member.

> >         __u32           size_sfinfo_kernel;		/* must be 0, set by kernel */
> >         __u32           size_sfinfo_user;
> >         __u32           size_tcpinfo_kernel;            /* must be 0, set by kernel */
> >         __u32           size_tcpinfo_user;
> 
> Do we need to have the kernel and user size in different fields?
> Is it not enough have:
> 
>   num_subflows;          /* will be set by the kernel */
>   num_subflows_copied;   /* max for the user, kernel put what it did */
>   size_sfinfo;           /* user set it, kernel put what it used: min */
>   size_tcpinfo;          /* same */

Yes, that would work too: kernel updates value to what its using.

I have no preference.  I used extra fields to better document
that this is filled in by the kernel.

I'll let Paolo make the call.

> If we add mptcp_info structure as mentioned by Paolo, we are getting
> very close the the getsockopt(MPTCP_INFO) from the fork kernel, see the
> section "Get detailed information of your subflows":
> 
> https://multipath-tcp.org/pmwiki.php/Users/ConfigureMPTCP
> 
> And the kernel header:
> 
> https://github.com/multipath-tcp/mptcp/blob/mptcp_trunk/include/uapi/linux/tcp.h#L321-L366

Right, this is similar.

> If we want to be even more future prove, why not having something more
> flexible where the userspace puts flags of what it wants and then there
> is a kind of TLV structure? Maybe it would be too complex to read/write?
> 
> 
>   struct mptcp_tlv_info {
>       __u32 type;
>       __u32 length;
>       __aligned_u64 value;
>   };
> 
>   struct mptcp_full_info {
>       __u32 flags;
>       __u32 num_subflows;
>       __u32 num_subflows_copied;
>       struct mptcp_tlv_info[];   /* number has to be linked to flags */
>   };

Hmm, I think it would be better to reuse an existing scheme
and e.g. return a netlink attribute blob in such a case,
this would also allow sharing with the diag interface/functions.