On 12/02/2016 09:36 AM, fx IWATA NOBUO wrote: > Hello, > >>> -static int do_accept(int listenfd) >>> +static int do_accept(int listenfd, char *host, char *port) > >> In my opinion you should pass also the size of those arrays to this >> function instead of hardcoding them like this. > > OK. I will add them. > >>> - if (usbip_driver_open(driver)) >>> + if (usbip_open_driver()) >> >> What's the purpose of this function? > >>> - usbip_driver_close(driver); >>> + usbip_close_driver(); >> >> As above? Why not make this a global variable and leave those calls as >> they were? > > usbip_driver_open/close - host side abstraction including stub and vudc. > > usbip_open/close_driver - abstraction both host(stub/vudc) and vhci. > > usbip_driver_<something> has widely used as function names and file names > for host side abstraction. > If they were usbip_host_<something>, I can use usbip_driver_open/close > for both host(stub/vudc) and vhci. usbip_host<sth>() is not correct name to use for both stub and vudc as from USB point of view stub is on a host but vudc is on a device side maybe a kind of usbipd_backed_init() would be more suitable? > >> I totally don't understand why those functions exist? >> Their names are very confusing and they don't implement any additional >> functionality. > > The abstraction is shared with patch 7/10. > >>> - driver = &device_driver; >>> + usbip_update_driver(); >> >> usbip_update_driver() is t totally unrelated to what this assignment >> really does. >> >> It changes the backend from stub driver to vudc not updating the driver. > > It used to make symmetric device side and application side. > In device side, it switches to device driver. > In application side, no operation. > So as above. I suggest to call it usbipd_backend() instead of driver. >> usbip_dev.c is not a best name for this file. It causes confusion (at least >> to me) as I don't know to what the dev part is related, usb device, device >> (stub side) or what? > > Device side. It's misleading as in USB we have host and device and this file is used on both (stub and vudc). > >>> + memset(&req, 0, sizeof(req)); >>> + memset(&reply, 0, sizeof(reply)); >> >> You don't need to 0 the reply. > > This is moved from usbipd.c. > I will remove. > >>> + PACK_OP_DEVLIST_REPLY(1, &reply); >> Could you please make the defines for 1 and 0 in this macro? >> As far as I understand the code it means pack and unpack but it would >> be much more readable if we have a define for this. > > Here's the list in the original code. > > usbip_attach.c: PACK_OP_IMPORT_REQUEST(0, &request); > usbip_attach.c: PACK_OP_IMPORT_REPLY(0, &reply); > usbip_list.c: PACK_OP_DEVLIST_REPLY(0, &reply); > usbip_network.c: PACK_OP_COMMON(1, &op_common); > usbip_network.c: PACK_OP_COMMON(0, &op_common); > usbipd.c: PACK_OP_IMPORT_REQUEST(0, &req); > usbipd.c: PACK_OP_DEVLIST_REPLY(1, &reply); > > Could I fix them in a refactoring patch including memset(0) and etc > in files tot touched in this set or outside of this set? > > PACK_OP_PACK 1 > PACK_OP_UNPACK 0 > Please do this outside this set as it's totally unrelated and just mark this series as depending on that change. It's a good idea to change this in all files not only in those you touch. Best regards, -- Krzysztof Opasiak Samsung R&D Institute Poland Samsung Electronics -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
