On 01/23/2019 03:24 PM, Björn Töpel wrote:
> Den ons 23 jan. 2019 kl 14:19 skrev Daniel Borkmann <dan...@iogearbox.net>:
>>
>> On 01/18/2019 02:03 PM, bjorn.to...@gmail.com wrote:
>>> From: Björn Töpel <bjorn.to...@intel.com>
>>>
>>> This patch adds the sock_diag interface for querying sockets from user
>>> space. Tools like iproute2 ss(8) can use this interface to list open
>>> AF_XDP sockets.
>>>
>>> The user-space ABI is defined in linux/xdp_diag.h and includes netlink
>>> request and response structs. The request can query sockets and the
>>> response contains socket information about the rings, umems, inode and
>>> more.
>>>
>>> Signed-off-by: Björn Töpel <bjorn.to...@intel.com>
>>
>> Series looks good, few minor nits inline:
>>
>>> ---
>>> include/uapi/linux/xdp_diag.h | 72 +++++++++++++
>>> net/xdp/Kconfig | 8 ++
>>> net/xdp/Makefile | 1 +
>>> net/xdp/xsk.c | 6 +-
>>> net/xdp/xsk.h | 12 +++
>>> net/xdp/xsk_diag.c | 192 ++++++++++++++++++++++++++++++++++
>>> 6 files changed, 286 insertions(+), 5 deletions(-)
>>> create mode 100644 include/uapi/linux/xdp_diag.h
>>> create mode 100644 net/xdp/xsk.h
>>> create mode 100644 net/xdp/xsk_diag.c
>>>
>>> diff --git a/include/uapi/linux/xdp_diag.h b/include/uapi/linux/xdp_diag.h
>>> new file mode 100644
>>> index 000000000000..efe8ce281dce
>>> --- /dev/null
>>> +++ b/include/uapi/linux/xdp_diag.h
>>> @@ -0,0 +1,72 @@
>>> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
>>> +/*
>>> + * xdp_diag: interface for query/monitor XDP sockets
>>> + * Copyright(c) 2019 Intel Corporation.
>>> + */
>>> +
>>> +#ifndef _LINUX_XDP_DIAG_H
>>> +#define _LINUX_XDP_DIAG_H
>>> +
>>> +#include <linux/types.h>
>>> +
>>> +struct xdp_diag_req {
>>> + __u8 sdiag_family;
>>> + __u8 sdiag_protocol;
>>> + __u16 pad;
>>
>> Presumably this one is for future use? Maybe better as '__u16 :16;' to
>> avoid compile errors if someone tries to zero 'pad' member manually?
>
> The "pad" was there simply to have an explicitly named structure hole.
> I'm not following the bitfield argument. Why does that avoid compiler
> errors?
Mostly in the sense that an application would explicitly set 'pad = 0'
whereas pad could later on potentially be renamed and reused otherwise
(which __u16 :16 would avoid in first place). But looking at other
*_diag_req structs, it's explicitly named as 'pad' elsewhere already,
then nevermind, lets have it rather consistent then and keep as is.
One small thing I still spotted when looking at it again, in function
xsk_diag_handler_dump() the req is unused.
Thanks,
Daniel
