Re: [PATCH v2] usbip: tools: usbipd: use ARRAY_SIZE for sockfdlist

[Shuah Khan]

On 6/11/21 4:39 AM, Greg KH wrote:
> On Fri, Jun 11, 2021 at 06:15:10PM +0800, Jiapeng Chong wrote:
> > Use ARRAY_SIZE instead of dividing sizeof array with sizeof an
> > element.
> >
> > Clean up the following coccicheck warning:
> >
> > ./tools/usb/usbip/src/usbipd.c:536:19-20: WARNING: Use ARRAY_SIZE.
>
> Why are you assuming that coccicheck should be run on userspace code?
>
> > Reported-by: Abaci Robot <abaci@xxxxxxxxxxxxxxxxx>
> > Signed-off-by: Jiapeng Chong <jiapeng.chong@xxxxxxxxxxxxxxxxx>
> > ---
> > Changes in v2:
> >    -Add ARRAY_SIZE definition to usbip_common.h file.
> >
> >   tools/usb/usbip/libsrc/usbip_common.h | 2 ++
> >   tools/usb/usbip/src/usbipd.c          | 3 +--
> >   2 files changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/tools/usb/usbip/libsrc/usbip_common.h b/tools/usb/usbip/libsrc/usbip_common.h
> > index 73a367a..4e12dc4 100644
> > --- a/tools/usb/usbip/libsrc/usbip_common.h
> > +++ b/tools/usb/usbip/libsrc/usbip_common.h
> > @@ -101,6 +101,8 @@
> >   		abort();				\
> >   	} while (0)
> >   
> > +#define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]))
>
> Why is this needed?
>
> And if it really really is, why not define it in a way that ALL tools
> can use it.
>
> And then fix it up be correct for cases when you might call this when it
> is not an array.  This is a very naive implementation.
>
> > +
> >   struct usbip_usb_interface {
> >   	uint8_t bInterfaceClass;
> >   	uint8_t bInterfaceSubClass;
> > diff --git a/tools/usb/usbip/src/usbipd.c b/tools/usb/usbip/src/usbipd.c
> > index 48398a7..4826d13 100644
> > --- a/tools/usb/usbip/src/usbipd.c
> > +++ b/tools/usb/usbip/src/usbipd.c
> > @@ -532,8 +532,7 @@ static int do_standalone_mode(int daemonize, int ipv4, int ipv6)
> >   		usbip_driver_close(driver);
> >   		return -1;
> >   	}
> > -	nsockfd = listen_all_addrinfo(ai_head, sockfdlist,
> > -		sizeof(sockfdlist) / sizeof(*sockfdlist));
> > +	nsockfd = listen_all_addrinfo(ai_head, sockfdlist, ARRAY_SIZE(sockfdlist));
>
> The code here is correct, right?  So this is not necessary unless you do
> this for all in-tree userspace tools at the same time.

A quick search shows me 53 defines for this across the kernel. Several
tools are defining this in their header scope as well as in .c files.

There is one in tools/include/linux/kernel.h - the tools that can be
compiled from tools/Makefile are okay. However, we have several tools
that aren't hooked into tools/Makefile and usbip is one of them. Some
tools maintainers probably don't want to add dependency on in-tree
header file so they can build it out of tree as a package. At least
that might be motivation behind adding the define under the specific
tool scope. Not true for usbip - it is something that was overlooked
when it was moved from staging to tools.

It is a good call to not add one more under this tool header scope
and do the cleanup for this properly which would include getting rid
of these duplicate defines that are in the kernel now.

thanks,
-- Shuah