Re: [PATCH v13 03/10] usbip: exporting devices: new connect operation

[Krzysztof Opasiak]

On 12/02/2016 08:05 AM, fx IWATA NOBUO wrote:
> Hello,
> 
>> In my humble opinion connect is not a best name for this operation.
> 
> Yes. I think connect is not perfect.
> 
>> it not only starts the connection to a remote server but also> exports
>> a device. I think that a name like export or attach_to_remote would be
>> more suitable. (...)
> 
> I think one word name is better.
> 
> Considering that attach is corresponding to import request, I think
> export is not good. Furthermore, export is already used in many places
> of the code, ex. usbip_host_common.c: usbip_export_device(). So I don't
> want to use export.
> 
> Operation | PDU | Description
> --+-+-
> attach|import   |invite a device
> detach|NA   |quit invited
> ( )   |export   |dedicate a device
> ( )   |un-export|quit dedicated
> 
> I think connect/disconnect is not bad.
> I will change it if there's user friendly, well-known and suit with
> existing name.
> 

Ehh, as usually the non technical problems are the toughest one;)

I would be happy to suggest you a good name for this but I also cannot
figure out any;)

connect was not perfect from my point of view as it suggest that we are
only establishing a tcp connection not connecting particular device but
maybe it's only mine problem and misunderstanding;)

Best regards,
-- 
Krzysztof Opasiak
Samsung R 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 03/10] usbip: exporting devices: new connect operation

[fx IWATA NOBUO]

Hello,

> In my humble opinion connect is not a best name for this operation.

Yes. I think connect is not perfect.

> it not only starts the connection to a remote server but also> exports
> a device. I think that a name like export or attach_to_remote would be
> more suitable. (...)

I think one word name is better.

Considering that attach is corresponding to import request, I think
export is not good. Furthermore, export is already used in many places
of the code, ex. usbip_host_common.c: usbip_export_device(). So I don't
want to use export.

Operation | PDU | Description
--+-+-
attach|import   |invite a device
detach|NA   |quit invited
( )   |export   |dedicate a device
( )   |un-export|quit dedicated

I think connect/disconnect is not bad.
I will change it if there's user friendly, well-known and suit with
existing name.

> > +   memset(, 0, sizeof(request));
> > +   memset(, 0, sizeof(reply));
> 
> Probably you don't need to 0 the reply as it is buffer for receiving
> and it's not being send anywhere.

I will remove it.
I will remove from send_unexport_device() in 4/10 too.

usbip_attach.c:query_import_device() has same memset.
I will fix it outside of this patch set.

> > +   memcpy(, udev, sizeof(struct usbip_usb_device));
> 
> how about sizeof(request.udev)?

I will change it.

I will grep sizeof and change to fit to nearby them.

> > +   /* check the reply */
> > +   if (reply.returncode) {
> > +   err("recv error return %d", reply.returncode);
> > +   return -1;
> > +   }
> 
> This check should probably look different or at least should be
> documented. Please refer to my comments to patch #1.

It will be removed.

> Take a look at all above function calls and your if.
> 
> You have usbip_driver_close() called 4 times in 4 error path. This is
> just asking to replace this with goto and place error path below
> return just like you did one function below.

I will fix it.

usbip_attach.c has same logic so I will add refactoring patch to
usbip_attach.c. regarding memset() and multiple driver_close.

Thank you for your help,

n.iwata
//
> -Original Message-
> From: Krzysztof Opasiak [mailto:k.opas...@samsung.com]
> Sent: Friday, December 02, 2016 5:28 AM
> To: fx IWATA NOBUO; valentina.mane...@gmail.com; shuah...@samsung.com;
> gre...@linuxfoundation.org; linux-usb@vger.kernel.org
> Cc: fx MICHIMURA TADAO
> Subject: Re: [PATCH v13 03/10] usbip: exporting devices: new connect
> operation
> 
> Hi,
> 
> On 11/22/2016 07:48 AM, Nobuo Iwata wrote:
> > Implementation of new connect operation. This is linked as a part of
> > usbip commnad. With this patch, usbip command has following operations.
> >
> >  bind
> >  unbind
> >  list (local/remote)
> >  attach
> >  detach
> >  port
> >  connect ... this patch
> 
> 
> In my humble opinion connect is not a best name for this operation.
> it not only starts the connection to a remote server but also exports a
> device. I think that a name like export or attach_to_remote would be more
> suitable.
> (...)
> 
> > +static const char usbip_connect_usage_string[] =
> > +   "usbip connect \n"
> > +   "-r, --remote=Address of a remote computer\n"
> > +   "-b, --busid=Bus ID of a device to be connected\n"
> > +   "-d, --device   Run with an alternate driver, e.g.
> vUDC\n";
> > +
> > +void usbip_connect_usage(void)
> > +{
> > +   printf("usage: %s", usbip_connect_usage_string); }
> > +
> > +static int send_export_device(int sockfd, struct usbip_usb_device
> > +*udev) {
> > +   int rc;
> > +   struct op_export_request request;
> > +   struct op_export_reply   reply;
> > +   uint16_t code = OP_REP_EXPORT;
> > +
> > +   memset(, 0, sizeof(request));
> > +   memset(, 0, sizeof(reply));
> 
> Probably you don't need to 0 the reply as it is buffer for receiving and
> it's not being send anywhere.
> 
> > +
> > +   /* send a request */
> > +   rc = usbip_net_send_op_common(sockfd, OP_REQ_EXPORT, 0);
> > +   if (rc < 0) {
> > +   err("send op_common");
> > +   return -1;
> > +   }
> > +
> > +   memcpy(, udev, sizeof(struct usbip_usb_device));
> 
> how about sizeof(request.udev)?
> 
> > +
> > +   PACK_OP_EXPORT_REQUEST(0, );
> > +
> > +   rc = usbip_net_send(sockfd, (void *) , sizeof(request));
> > +   if (rc < 0) {
> > +   err("send op_export_request");
> > +   return -1;
> > +   }
> 

Re: [PATCH v13 03/10] usbip: exporting devices: new connect operation

[Krzysztof Opasiak]

Hi,

On 11/22/2016 07:48 AM, Nobuo Iwata wrote:
> Implementation of new connect operation. This is linked as a part of 
> usbip commnad. With this patch, usbip command has following operations.
> 
>  bind
>  unbind
>  list (local/remote)
>  attach
>  detach
>  port
>  connect ... this patch


In my humble opinion connect is not a best name for this operation.
it not only starts the connection to a remote server but also exports a
device. I think that a name like export or attach_to_remote would be
more suitable.
(...)

> +static const char usbip_connect_usage_string[] =
> + "usbip connect \n"
> + "-r, --remote=Address of a remote computer\n"
> + "-b, --busid=Bus ID of a device to be connected\n"
> + "-d, --device   Run with an alternate driver, e.g. vUDC\n";
> +
> +void usbip_connect_usage(void)
> +{
> + printf("usage: %s", usbip_connect_usage_string);
> +}
> +
> +static int send_export_device(int sockfd, struct usbip_usb_device *udev)
> +{
> + int rc;
> + struct op_export_request request;
> + struct op_export_reply   reply;
> + uint16_t code = OP_REP_EXPORT;
> +
> + memset(, 0, sizeof(request));
> + memset(, 0, sizeof(reply));

Probably you don't need to 0 the reply as it is buffer for receiving and
it's not being send anywhere.

> +
> + /* send a request */
> + rc = usbip_net_send_op_common(sockfd, OP_REQ_EXPORT, 0);
> + if (rc < 0) {
> + err("send op_common");
> + return -1;
> + }
> +
> + memcpy(, udev, sizeof(struct usbip_usb_device));

how about sizeof(request.udev)?

> +
> + PACK_OP_EXPORT_REQUEST(0, );
> +
> + rc = usbip_net_send(sockfd, (void *) , sizeof(request));
> + if (rc < 0) {
> + err("send op_export_request");
> + return -1;
> + }
> +
> + /* receive a reply */
> + rc = usbip_net_recv_op_common(sockfd, );
> + if (rc < 0) {
> + err("recv op_common");
> + return -1;
> + }
> +
> + rc = usbip_net_recv(sockfd, (void *) , sizeof(reply));
> + if (rc < 0) {
> + err("recv op_export_reply");
> + return -1;
> + }
> +
> + PACK_OP_EXPORT_REPLY(0, );
> +
> + /* check the reply */
> + if (reply.returncode) {
> + err("recv error return %d", reply.returncode);
> + return -1;
> + }

This check should probably look different or at least should be
documented. Please refer to my comments to patch #1.

> +
> + return 0;
> +}
> +
> +static int export_device(char *busid, int sockfd)
> +{
> + int rc;
> + struct usbip_exported_device *edev;
> +
> + rc = usbip_driver_open(driver);
> + if (rc) {
> + err("open driver");
> + return -1;
> + }
> +
> + rc = usbip_refresh_device_list(driver);
> + if (rc < 0) {
> + err("could not refresh device list");
> + usbip_driver_close(driver);
> + return -1;
> + }
> +
> + edev = usbip_get_device(driver, busid);
> + if (edev == NULL) {
> + err("find device");
> + usbip_driver_close(driver);
> + return -1;
> + }
> +
> + rc = send_export_device(sockfd, >udev);
> + if (rc < 0) {
> + err("send export");
> + usbip_driver_close(driver);
> + return -1;
> + }
> +
> + rc = usbip_export_device(edev, sockfd);
> + if (rc < 0) {
> + err("export device");
> + usbip_driver_close(driver);
> + return -1;
> + }

Take a look at all above function calls and your if.

You have usbip_driver_close() called 4 times in 4 error path. This is
just asking to replace this with goto and place error path below return
just like you did one function below.

Not to mention that you should propagate the error code to caller.

> +
> + usbip_driver_close(driver);
> +
> + return 0;
> +}

Best regards,
-- 
Krzysztof Opasiak
Samsung R 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 03/10] usbip: exporting devices: new connect operation

[fx IWATA NOBUO]

Hello,

> Can client use the same import over vpn connection

Yes.
Using VPN, the firewall problem doesn't need to be concerned.

To build proprietary virtual network in internet, VPN can be used.
The goal #1 will give flexibility for various kind of internet or
cloud service.

Using VPN, goal #2 "To improve usability of operations" can be useful.

> or some sort of authorized mechanism to access to the server?

Yes.
USB/IP already has TCP wrapper option.
When using with SSL or WebSocket(wss) tunneling proxy, client and
Server authentication can be used.

> I want know a clear use-case definition of why the current client pull
> model to import devices doesn't work. I understand firewalls pose some
> issues. However, it doesn't sound right that server initiates export
> and contacts the client.

Does the response to 00/00 comments and answers above make clear?

> What happens when client doesn't have access to the information on the
> server.

I couldn't catch this question especially 'the information'.

> Also the above doesn't look right without email address.

OK. I will add email address.

> Please don't add new Copyright to an existing file.

Do you mean not to add to existing files if it doesn't have certain
changes?

If so, I will check and fix again.

Here's an example of copyright in a file.

 * Copyright (C) 2011 matt mooney <m...@muteddisk.com>
 *   2005-2007 Takahiro Hirofuchi
 * Copyright (C) 2015-2016 Samsung Electronics
 *   Igor Kotrasinski <i.kotrasi...@samsung.com>
 *   Krzysztof Opasiak <k.opas...@samsung.com>

Ver.1 of this patch set is older than Samsung's vudc patches so I added
above regarding the year order.

Please, let me know if it's not good. I will move it below.

Thank you,

nobuo.iwata
//
> -Original Message-
> From: Shuah Khan [mailto:shuah...@samsung.com]
> Sent: Thursday, November 24, 2016 6:52 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 03/10] usbip: exporting devices: new connect
> operation
> 
> On 11/21/2016 11:48 PM, Nobuo Iwata wrote:
> > Implementation of new connect operation. This is linked as a part of
> > usbip commnad. With this patch, usbip command has following operations.
> >
> >  bind
> >  unbind
> >  list (local/remote)
> >  attach
> >  detach
> >  port
> >  connect ... this patch
> 
> I am finding it hard to follow these changes without the server and client
> side details. A few comments below:
> 
> I want know a clear use-case definition of why the current client pull model
> to import devices doesn't work. I understand firewalls pose some issues.
> However, it doesn't sound right that server initiates export and contacts
> the client. What happens when client doesn't have access to the information
> on the server.
> 
> Can client use the same import over vpn connection or some sort of authorized
> mechanism to access to the server?
> 
> >
> > In device side node, this binds a device internally, sends an export
> > request and receives export response. The definition of the request
> > and response are defined in original code:
> > tools/usb/usbip/usbip_network.h but was not used. They are corresponding
> to NEW-3 in following diagram.
> >
> > 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 --->
> >
> > Signed-off-by: Nobuo Iwata <nobuo.iw...@fujixerox.co.jp>
> > ---
> >  tools/usb/usbip/src/Makefile.am |   3 +-
> >  tools/usb/usbip/src/usbip.c |   9 +-
> >  tools/usb/usbip/src/usbip.h |   5

Re: [PATCH v13 03/10] usbip: exporting devices: new connect operation

[Shuah Khan]

On 11/21/2016 11:48 PM, Nobuo Iwata wrote:
> Implementation of new connect operation. This is linked as a part of 
> usbip commnad. With this patch, usbip command has following operations.
> 
>  bind
>  unbind
>  list (local/remote)
>  attach
>  detach
>  port
>  connect ... this patch

I am finding it hard to follow these changes without the server and client
side details. A few comments below:

I want know a clear use-case definition of why the current client pull
model to import devices doesn't work. I understand firewalls pose some
issues. However, it doesn't sound right that server initiates export
and contacts the client. What happens when client doesn't have access
to the information on the server.

Can client use the same import over vpn connection or some sort of
authorized mechanism to access to the server?

> 
> In device side node, this binds a device internally, sends an export 
> request and receives export response. The definition of the request and 
> response are defined in original code: tools/usb/usbip/usbip_network.h 
> but was not used. They are corresponding to NEW-3 in following diagram. 
> 
> 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 --->
> 
> Signed-off-by: Nobuo Iwata 
> ---
>  tools/usb/usbip/src/Makefile.am |   3 +-
>  tools/usb/usbip/src/usbip.c |   9 +-
>  tools/usb/usbip/src/usbip.h |   5 +-
>  tools/usb/usbip/src/usbip_connect.c | 228 
>  4 files changed, 242 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/usb/usbip/src/Makefile.am b/tools/usb/usbip/src/Makefile.am
> index e81a4eb..0947476 100644
> --- a/tools/usb/usbip/src/Makefile.am
> +++ b/tools/usb/usbip/src/Makefile.am
> @@ -6,6 +6,7 @@ sbin_PROGRAMS := usbip usbipd
>  
>  usbip_SOURCES := usbip.h utils.h usbip.c utils.c usbip_network.c \
>usbip_attach.c usbip_detach.c usbip_list.c \
> -  usbip_bind.c usbip_unbind.c usbip_port.c
> +  usbip_bind.c usbip_unbind.c usbip_port.c \
> +  usbip_connect.c
>  
>  usbipd_SOURCES := usbip_network.h usbipd.c usbip_network.c
> diff --git a/tools/usb/usbip/src/usbip.c b/tools/usb/usbip/src/usbip.c
> index d7599d9..584d7d5 100644
> --- a/tools/usb/usbip/src/usbip.c
> +++ b/tools/usb/usbip/src/usbip.c
> @@ -2,7 +2,8 @@
>   * command structure borrowed from udev
>   * (git://git.kernel.org/pub/scm/linux/hotplug/udev.git)
>   *
> - * Copyright (C) 2011 matt mooney 
> + * Copyright (C) 2015 Nobuo Iwata
> + *   2011 matt mooney 
>   *   2005-2007 Takahiro Hirofuchi
>   *
>   * This program is free software: you can redistribute it and/or modify
> @@ -76,6 +77,12 @@ static const struct command cmds[] = {
>   .usage = usbip_detach_usage
>   },
>   {
> + .name  = "connect",
> + .fn= usbip_connect,
> + .help  = "Connect a USB device to a remote computer",
> + .usage = usbip_connect_usage
> + },
> + {
>   .name  = "list",
>   .fn= usbip_list,
>   .help  = "List exportable or local USB devices",
> diff --git a/tools/usb/usbip/src/usbip.h b/tools/usb/usbip/src/usbip.h
> index c296910..f365353 100644
> --- a/tools/usb/usbip/src/usbip.h
> +++ b/tools/usb/usbip/src/usbip.h
> @@ -1,5 +1,6 @@
>  /*
> - * Copyright (C) 2011 matt mooney 
> + * Copyright (C) 2015 Nobuo Iwata

Please don't add new Copyright to an existing file. Also the above
doesn't look right without email address. Please treat this as a
global comment for the Copyright changes in this patch series.


> + *   2011 matt mooney 
>   *   2005-2007 Takahiro Hirofuchi
>   *
>   * This program is free software: you can redistribute it and/or modify
> @@ -30,12 +31,14 @@ int usbip_list(int argc, char *argv[]);
>  int usbip_bind(int argc, char 

[PATCH v13 03/10] usbip: exporting devices: new connect operation

[Nobuo Iwata]

Implementation of new connect operation. This is linked as a part of 
usbip commnad. With this patch, usbip command has following operations.

 bind
 unbind
 list (local/remote)
 attach
 detach
 port
 connect ... this patch

In device side node, this binds a device internally, sends an export 
request and receives export response. The definition of the request and 
response are defined in original code: tools/usb/usbip/usbip_network.h 
but was not used. They are corresponding to NEW-3 in following diagram. 

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 --->

Signed-off-by: Nobuo Iwata 
---
 tools/usb/usbip/src/Makefile.am |   3 +-
 tools/usb/usbip/src/usbip.c |   9 +-
 tools/usb/usbip/src/usbip.h |   5 +-
 tools/usb/usbip/src/usbip_connect.c | 228 
 4 files changed, 242 insertions(+), 3 deletions(-)

diff --git a/tools/usb/usbip/src/Makefile.am b/tools/usb/usbip/src/Makefile.am
index e81a4eb..0947476 100644
--- a/tools/usb/usbip/src/Makefile.am
+++ b/tools/usb/usbip/src/Makefile.am
@@ -6,6 +6,7 @@ sbin_PROGRAMS := usbip usbipd
 
 usbip_SOURCES := usbip.h utils.h usbip.c utils.c usbip_network.c \
 usbip_attach.c usbip_detach.c usbip_list.c \
-usbip_bind.c usbip_unbind.c usbip_port.c
+usbip_bind.c usbip_unbind.c usbip_port.c \
+usbip_connect.c
 
 usbipd_SOURCES := usbip_network.h usbipd.c usbip_network.c
diff --git a/tools/usb/usbip/src/usbip.c b/tools/usb/usbip/src/usbip.c
index d7599d9..584d7d5 100644
--- a/tools/usb/usbip/src/usbip.c
+++ b/tools/usb/usbip/src/usbip.c
@@ -2,7 +2,8 @@
  * command structure borrowed from udev
  * (git://git.kernel.org/pub/scm/linux/hotplug/udev.git)
  *
- * Copyright (C) 2011 matt mooney 
+ * Copyright (C) 2015 Nobuo Iwata
+ *   2011 matt mooney 
  *   2005-2007 Takahiro Hirofuchi
  *
  * This program is free software: you can redistribute it and/or modify
@@ -76,6 +77,12 @@ static const struct command cmds[] = {
.usage = usbip_detach_usage
},
{
+   .name  = "connect",
+   .fn= usbip_connect,
+   .help  = "Connect a USB device to a remote computer",
+   .usage = usbip_connect_usage
+   },
+   {
.name  = "list",
.fn= usbip_list,
.help  = "List exportable or local USB devices",
diff --git a/tools/usb/usbip/src/usbip.h b/tools/usb/usbip/src/usbip.h
index c296910..f365353 100644
--- a/tools/usb/usbip/src/usbip.h
+++ b/tools/usb/usbip/src/usbip.h
@@ -1,5 +1,6 @@
 /*
- * Copyright (C) 2011 matt mooney 
+ * Copyright (C) 2015 Nobuo Iwata
+ *   2011 matt mooney 
  *   2005-2007 Takahiro Hirofuchi
  *
  * This program is free software: you can redistribute it and/or modify
@@ -30,12 +31,14 @@ int usbip_list(int argc, char *argv[]);
 int usbip_bind(int argc, char *argv[]);
 int usbip_unbind(int argc, char *argv[]);
 int usbip_port_show(int argc, char *argv[]);
+int usbip_connect(int argc, char *argv[]);
 
 void usbip_attach_usage(void);
 void usbip_detach_usage(void);
 void usbip_list_usage(void);
 void usbip_bind_usage(void);
 void usbip_unbind_usage(void);
+void usbip_connect_usage(void);
 
 int usbip_bind_device(char *busid);
 int usbip_unbind_device(char *busid);
diff --git a/tools/usb/usbip/src/usbip_connect.c 
b/tools/usb/usbip/src/usbip_connect.c
new file mode 100644
index 000..bbecc7e
--- /dev/null
+++ b/tools/usb/usbip/src/usbip_connect.c
@@ -0,0 +1,228 @@
+/*
+ * Copyright (C) 2015 Nobuo Iwata
+ *   2011 matt mooney 
+ *   2005-2007 Takahiro Hirofuchi
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, either version 2