RE: [PATCH v13 02/10] usbip: exporting devices: modifications to host side libraries
Hello, > In my humble opinion the code itself is good. The problem is commit > message. Current commit massage has nothing to do with patch content > and should be totally changed. > > Please elaborate that this function is unused, write that using > indexes on a list is generally not a good idea and so on. OK. I will improve the commit message. Thank you, n.iwata // > -Original Message- > From: Krzysztof Opasiak [mailto:k.opas...@samsung.com] > Sent: Friday, December 02, 2016 4:57 AM > To: fx IWATA NOBUO; shuah...@samsung.com; valentina.mane...@gmail.com; > gre...@linuxfoundation.org; linux-usb@vger.kernel.org > Cc: fx MICHIMURA TADAO; Shuah Khan > Subject: Re: [PATCH v13 02/10] usbip: exporting devices: modifications to > host side libraries > > > > On 11/24/2016 06:59 AM, fx IWATA NOBUO wrote: > > Hello, > > > >> This doesn't look like a simple change to rename and reuse an unused > >> function. This patch does lot more and is changing the user interface. > >> Looks like instead of taking an integer value for device lookup, you > >> are changing it to char *. > >> > >> Any reason why you have to change the user interface to go to char > >> *busid? > >> > >> I would like to see a good explanation why this user interface change > >> is necessary. > > > > I can't figure out why the second argument was int because it was > > unused. > > > > I also cannot figure out this but using busid here looks much more reasonable > to me. > > > usbip_get_device() is a stub-side (usb-host) function. > > As you know, only port number in vhci-side can be int. > > Others are bus-id. > > Furthermore, the unused code searches list node position. It doesn't > > make sense. > > > > Agree. > > > Here, this patch set needs to find bound device with bus-id in > > stub-side. > > stub-side or vudc side. In case of vudc the busid is set to usbip-vudc.%d > > > > > I may add explanation above to change log. > > Or I can change new function name like usbip_find_device() and leave > > the unused function once. > > > > Please, let me know how should I do. > > In my humble opinion the code itself is good. The problem is commit message. > Current commit massage has nothing to do with patch content and should be > totally changed. > > Please elaborate that this function is unused, write that using indexes > on a list is generally not a good idea and so on. > > 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v13 02/10] usbip: exporting devices: modifications to host side libraries
On 11/24/2016 06:59 AM, fx IWATA NOBUO wrote: > Hello, > >> This doesn't look like a simple change to rename and reuse an unused >> function. This patch does lot more and is changing the user interface. >> Looks like instead of taking an integer value for device lookup, you >> are changing it to char *. >> >> Any reason why you have to change the user interface to go to char >> *busid? >> >> I would like to see a good explanation why this user interface change >> is necessary. > > I can't figure out why the second argument was int because it was > unused. > I also cannot figure out this but using busid here looks much more reasonable to me. > usbip_get_device() is a stub-side (usb-host) function. > As you know, only port number in vhci-side can be int. > Others are bus-id. > Furthermore, the unused code searches list node position. It doesn't > make sense. > Agree. > Here, this patch set needs to find bound device with bus-id in > stub-side. stub-side or vudc side. In case of vudc the busid is set to usbip-vudc.%d > > I may add explanation above to change log. > Or I can change new function name like usbip_find_device() and leave the > unused function once. > > Please, let me know how should I do. In my humble opinion the code itself is good. The problem is commit message. Current commit massage has nothing to do with patch content and should be totally changed. Please elaborate that this function is unused, write that using indexes on a list is generally not a good idea and so on. 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v13 02/10] usbip: exporting devices: modifications to host side libraries
Hello,
> This doesn't look like a simple change to rename and reuse an unused
> function. This patch does lot more and is changing the user interface.
> Looks like instead of taking an integer value for device lookup, you
> are changing it to char *.
>
> Any reason why you have to change the user interface to go to char
> *busid?
>
> I would like to see a good explanation why this user interface change
> is necessary.
I can't figure out why the second argument was int because it was
unused.
usbip_get_device() is a stub-side (usb-host) function.
As you know, only port number in vhci-side can be int.
Others are bus-id.
Furthermore, the unused code searches list node position. It doesn't
make sense.
Here, this patch set needs to find bound device with bus-id in
stub-side.
I may add explanation above to change log.
Or I can change new function name like usbip_find_device() and leave the
unused function once.
Please, let me know how should I do.
Best Regards,
nobuo.iwata
//
> -Original Message-
> From: Shuah Khan [mailto:shuah...@samsung.com]
> Sent: Thursday, November 24, 2016 6:02 AM
> To: fx IWATA NOBUO; valentina.mane...@gmail.com;
> gre...@linuxfoundation.org; linux-usb@vger.kernel.org
> Cc: fx MICHIMURA TADAO; Shuah Khan
> Subject: Re: [PATCH v13 02/10] usbip: exporting devices: modifications to
> host side libraries
>
> On 11/21/2016 11:48 PM, Nobuo Iwata wrote:
> > usbip_get_device() method in usbip_host_driver_ops was not used. It is
> > modified as a function to find an exported device for new operations
> > 'connect' and 'disconnect'.
> >
> > bind and unbind function are exported to reuse by new connect and
> > disconnect operation.
>
> This doesn't look like a simple change to rename and reuse an unused function.
> This patch does lot more and is changing the user interface.
> Looks like instead of taking an integer value for device lookup, you are
> changing it to char *.
>
> Any reason why you have to change the user interface to go to char *busid?
>
> I would like to see a good explanation why this user interface change is
> necessary.
>
> thanks,
> -- Shuah
>
> >
> > Here, connect and disconnect is NEW-3 and NEW-4 respactively in
> > diagram below.
> >
> > EXISTING) - invites devices from application(vhci)-side
> > +--+
> +--+
> > device--+ STUB | | application/VHCI |
> > +--+
> +--+
> > 1) usbipd ... start daemon
> > = = =
> > 2) usbip list --local
> > 3) usbip bind
> > <--- list bound devices --- 4) usbip list --remote
> > <--- import a device -- 5) usbip attach
> > = = =
> >X disconnected 6) usbip detach
> > 7) usbip unbind
> >
> > NEW) - dedicates devices from device(stb)-side
> > +--+
> +--+
> > device--+ STUB | | application/VHCI |
> > +--+
> +--+
> > 1) usbipa ... start
> > daemon = = =
> > 2) usbip list --local
> > 3) usbip connect --- export a device -->
> > = = =
> > 4) usbip disconnect --- un-export a device --->
> >
> > Bind and unbind are done in connect and disconnect internally.
> >
> > Signed-off-by: Nobuo Iwata
> > ---
> > tools/usb/usbip/libsrc/usbip_host_common.c | 6 ++
> > tools/usb/usbip/libsrc/usbip_host_common.h | 8
> > tools/usb/usbip/src/usbip.h| 3 +++
> > tools/usb/usbip/src/usbip_bind.c | 4 ++--
> > tools/usb/usbip/src/usbip_unbind.c | 4 ++--
> > 5 files changed, 13 insertions(+), 12 deletions(-)
> >
> > diff --git a/tools/usb/usbip/libsrc/usbip_host_common.c
> > b/tools/usb/usbip/libsrc/usbip_host_common.c
> > index 9d41522..6a98d6c 100644
> > --- a/tools/usb/usbip/libsrc/usbip_host_common.c
> > +++ b/tools/usb/usbip/libsrc/usbip_host_common.c
> > @@ -256,17 +256,15 @@ int usbip_export_device(struct
> > usbip_exported_device *edev, int sockfd) }
> >
> > struct usbip_exported_device *usbip_generic_get_device(
> > - struct usbip_host_driver *hdriver, int num)
> > + struct usbip_host_driver *hdriver, char *busid)
> > {
> > struct list_head *i;
> > struct usbip_exported_device *edev;
> > - int cnt = 0;
> >
> > list_for_each(i, &hdriver->edev_list) {
> > edev = list_entry(
Re: [PATCH v13 02/10] usbip: exporting devices: modifications to host side libraries
On 11/21/2016 11:48 PM, Nobuo Iwata wrote:
> usbip_get_device() method in usbip_host_driver_ops was not used. It is
> modified as a function to find an exported device for new operations
> 'connect' and 'disconnect'.
>
> bind and unbind function are exported to reuse by new connect and
> disconnect operation.
This doesn't look like a simple change to rename and reuse an unused
function. This patch does lot more and is changing the user interface.
Looks like instead of taking an integer value for device lookup, you
are changing it to char *.
Any reason why you have to change the user interface to go to char *busid?
I would like to see a good explanation why this user interface change is
necessary.
thanks,
-- Shuah
>
> Here, connect and disconnect is NEW-3 and NEW-4 respactively in diagram
> below.
>
> EXISTING) - invites devices from application(vhci)-side
> +--+ +--+
> device--+ STUB | | application/VHCI |
> +--+ +--+
> 1) usbipd ... start daemon
> = = =
> 2) usbip list --local
> 3) usbip bind
> <--- list bound devices --- 4) usbip list --remote
> <--- import a device -- 5) usbip attach
> = = =
>X disconnected 6) usbip detach
> 7) usbip unbind
>
> NEW) - dedicates devices from device(stb)-side
> +--+ +--+
> device--+ STUB | | application/VHCI |
> +--+ +--+
> 1) usbipa ... start daemon
> = = =
> 2) usbip list --local
> 3) usbip connect --- export a device -->
> = = =
> 4) usbip disconnect --- un-export a device --->
>
> Bind and unbind are done in connect and disconnect internally.
>
> Signed-off-by: Nobuo Iwata
> ---
> tools/usb/usbip/libsrc/usbip_host_common.c | 6 ++
> tools/usb/usbip/libsrc/usbip_host_common.h | 8
> tools/usb/usbip/src/usbip.h| 3 +++
> tools/usb/usbip/src/usbip_bind.c | 4 ++--
> tools/usb/usbip/src/usbip_unbind.c | 4 ++--
> 5 files changed, 13 insertions(+), 12 deletions(-)
>
> diff --git a/tools/usb/usbip/libsrc/usbip_host_common.c
> b/tools/usb/usbip/libsrc/usbip_host_common.c
> index 9d41522..6a98d6c 100644
> --- a/tools/usb/usbip/libsrc/usbip_host_common.c
> +++ b/tools/usb/usbip/libsrc/usbip_host_common.c
> @@ -256,17 +256,15 @@ int usbip_export_device(struct usbip_exported_device
> *edev, int sockfd)
> }
>
> struct usbip_exported_device *usbip_generic_get_device(
> - struct usbip_host_driver *hdriver, int num)
> + struct usbip_host_driver *hdriver, char *busid)
> {
> struct list_head *i;
> struct usbip_exported_device *edev;
> - int cnt = 0;
>
> list_for_each(i, &hdriver->edev_list) {
> edev = list_entry(i, struct usbip_exported_device, node);
> - if (num == cnt)
> + if (!strncmp(busid, edev->udev.busid, SYSFS_BUS_ID_SIZE))
> return edev;
> - cnt++;
> }
>
> return NULL;
> diff --git a/tools/usb/usbip/libsrc/usbip_host_common.h
> b/tools/usb/usbip/libsrc/usbip_host_common.h
> index a64b803..f9a9def 100644
> --- a/tools/usb/usbip/libsrc/usbip_host_common.h
> +++ b/tools/usb/usbip/libsrc/usbip_host_common.h
> @@ -38,7 +38,7 @@ struct usbip_host_driver_ops {
> void (*close)(struct usbip_host_driver *hdriver);
> int (*refresh_device_list)(struct usbip_host_driver *hdriver);
> struct usbip_exported_device * (*get_device)(
> - struct usbip_host_driver *hdriver, int num);
> + struct usbip_host_driver *hdriver, char *busid);
>
> int (*read_device)(struct udev_device *sdev,
> struct usbip_usb_device *dev);
> @@ -86,11 +86,11 @@ static inline int usbip_refresh_device_list(struct
> usbip_host_driver *hdriver)
> }
>
> static inline struct usbip_exported_device *
> -usbip_get_device(struct usbip_host_driver *hdriver, int num)
> +usbip_get_device(struct usbip_host_driver *hdriver, char *busid)
> {
> if (!hdriver->ops.get_device)
> return NULL;
> - return hdriver->ops.get_device(hdriver, num);
> + return hdriver->ops.get_device(hdriver, busid);
> }
>
> /* Helper functions for implementing driver backend */
> @@ -99,6 +99,6 @@ void usbip_generic_driver_close(struct usbip_host_driver
> *hdriver);
> int usbip_generic_refresh_device_list(struct usbip_host_driver *hdriver);
> int usbip_export_device(struct usbip_exported_device *edev, int sockfd);
> struct usbip_exported_device *usbip_generic_get_device(
> - struct usbip_host_driver *hdriver, int num);
> + struct usbip_host_driver *hdriver, char *busid);
