Hello, > > 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 OK. > maybe a kind of usbipd_backed_init() would be more suitable? Naming problem again but I recognize usbip_open_driver() is worse than connect. I think the word 'backend' has wide meaning and more strict word is better. init driver = &host_driver; NA ( ) driver = &device_driver; NA open usbip_driver_open usbip_vhci_driver_open close usbip_driver_close usbip_vhci_driver_close Here, I'd like to use word 'driver'. I searched analogy meta_, super_ in kernel. How about usbip_meta_driver_init/open/close()? > >> usbip_update_driver() is t totally unrelated to what this assignment > >> really does. > So as above. I suggest to call it usbipd_backend() instead of driver. Please, let me know good verb representing 'driver = &device_driver'. > >>> + 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. I found the 'reply' is not used so I will remove the definition too. > > 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. OK. I will. Thank you for your help, n.iwata // > -----Original Message----- > From: Krzysztof Opasiak [mailto:k.opasiak@xxxxxxxxxxx] > Sent: Friday, December 02, 2016 11:20 PM > To: fx IWATA NOBUO; valentina.manea.m@xxxxxxxxx; shuah.kh@xxxxxxxxxxx; > gregkh@xxxxxxxxxxxxxxxxxxx; linux-usb@xxxxxxxxxxxxxxx > Cc: fx MICHIMURA TADAO > Subject: Re: [PATCH v13 05/10] usbip: exporting devices: modifications to > daemon > > > > 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
