RE: [PATCH v13 04/10] usbip: exporting devices: new disconnect operation

[fx IWATA NOBUO]

> > +   memset(, 0, sizeof(request));
> > +   memset(, 0, sizeof(reply));
> 
> As in previous patch, you don't need to 0 the reply.

I will remove the memset.

> sizeof(request.udev)?

I will modify.

> > +   /* check the reply */
> > +   if (reply.returncode) {
> > +   err("recv error return %d", reply.returncode);
> > +   return -1;
> > +   }
> 
> The same case with error code as in previous patch.

It will be removed.

> Once again the same pattern as in previous patch 3 times in a row copy-pasted
> error path. Please use goto here and place error path below return.

I will fix it.

> close(sockfd) in case of error, otherwise close(sockfd)...
> 
> Probably you can place close(sockfd) above if to avoid this weird
> copy-paste.

I will fix it.

> > +   ret = disconnect_device(host, busid, unbind);
> > +   goto out;
> 
> This looks a little bit weird... why you use goto out instead of just return
> ret?

err_out has usbip_disconnect_usage().

Each usbip_.c has same pattern.

Thank you,

n.iwata
//
> -Original Message-
> From: Krzysztof Opasiak [mailto:k.opas...@samsung.com]
> Sent: Friday, December 02, 2016 5:37 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 04/10] usbip: exporting devices: new disconnect
> operation
> 
> 
> 
> On 11/22/2016 07:48 AM, Nobuo Iwata wrote:
> (...)
> > +
> > +static int send_unexport_device(int sockfd, struct usbip_usb_device
> > +*udev) {
> > +   int rc;
> > +   struct op_unexport_request request;
> > +   struct op_unexport_reply   reply;
> > +   uint16_t code = OP_REP_UNEXPORT;
> > +
> > +   memset(, 0, sizeof(request));
> > +   memset(, 0, sizeof(reply));
> 
> As in previous patch, you don't need to 0 the reply.
> 
> > +
> > +   /* send a request */
> > +   rc = usbip_net_send_op_common(sockfd, OP_REQ_UNEXPORT, 0);
> > +   if (rc < 0) {
> > +   err("send op_common");
> > +   return -1;
> > +   }
> > +
> > +   memcpy(, udev, sizeof(struct usbip_usb_device));
> 
> sizeof(request.udev)?
> 
> > +
> > +   PACK_OP_UNEXPORT_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_unexport_reply");
> > +   return -1;
> > +   }
> > +
> > +   PACK_OP_EXPORT_REPLY(0, );
> > +
> > +   /* check the reply */
> > +   if (reply.returncode) {
> > +   err("recv error return %d", reply.returncode);
> > +   return -1;
> > +   }
> 
> The same case with error code as in previous patch.
> 
> > +
> > +   return 0;
> > +}
> > +
> > +static int unexport_device(char *busid, int sockfd) {
> > +   int rc;
> > +   struct usbip_exported_device *edev;
> > +
> > +   rc = usbip_driver_open(driver);
> > +   if (rc < 0) {
> > +   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_unexport_device(sockfd, >udev);
> > +   if (rc < 0) {
> > +   err("send unexport");
> > +   usbip_driver_close(driver);
> > +   return -1;
> > +   }
> 
> Once again the same pattern as in previous patch 3 times in a row copy-pasted
> error path. Please use goto here and place error path below return.
> 
> > +
> > +   usbip_driver_close(driver);
> > +
> > +   return 0;
> > +}
> > +
> > +static int disconnect_de

Re: [PATCH v13 04/10] usbip: exporting devices: new disconnect operation

[Krzysztof Opasiak]

On 11/22/2016 07:48 AM, Nobuo Iwata wrote:
(...)
> +
> +static int send_unexport_device(int sockfd, struct usbip_usb_device *udev)
> +{
> + int rc;
> + struct op_unexport_request request;
> + struct op_unexport_reply   reply;
> + uint16_t code = OP_REP_UNEXPORT;
> +
> + memset(, 0, sizeof(request));
> + memset(, 0, sizeof(reply));

As in previous patch, you don't need to 0 the reply.

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

sizeof(request.udev)?

> +
> + PACK_OP_UNEXPORT_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_unexport_reply");
> + return -1;
> + }
> +
> + PACK_OP_EXPORT_REPLY(0, );
> +
> + /* check the reply */
> + if (reply.returncode) {
> + err("recv error return %d", reply.returncode);
> + return -1;
> + }

The same case with error code as in previous patch.

> +
> + return 0;
> +}
> +
> +static int unexport_device(char *busid, int sockfd)
> +{
> + int rc;
> + struct usbip_exported_device *edev;
> +
> + rc = usbip_driver_open(driver);
> + if (rc < 0) {
> + 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_unexport_device(sockfd, >udev);
> + if (rc < 0) {
> + err("send unexport");
> + usbip_driver_close(driver);
> + return -1;
> + }

Once again the same pattern as in previous patch 3 times in a row
copy-pasted error path. Please use goto here and place error path below
return.

> +
> + usbip_driver_close(driver);
> +
> + return 0;
> +}
> +
> +static int disconnect_device(char *host, char *busid, int unbind)
> +{
> + int sockfd;
> + int rc;
> +
> + sockfd = usbip_net_tcp_connect(host, usbip_port_string);
> + if (sockfd < 0) {
> + err("tcp connect");
> + return -1;
> + }
> +
> + rc = unexport_device(busid, sockfd);
> + if (rc < 0) {
> + err("unexport");
> + close(sockfd);
> + return -1;
> + }

close(sockfd) in case of error, otherwise close(sockfd)...

Probably you can place close(sockfd) above if to avoid this weird
copy-paste.

> +
> + close(sockfd);
> +
> + if (unbind) {
> + rc = usbip_unbind_device(busid);
> + if (rc) {
> + err("unbind");
> + return -1;
> + }
> + }
> +
> + return 0;
> +}
> +
> +int usbip_disconnect(int argc, char *argv[])
> +{
> + static const struct option opts[] = {
> + { "remote", required_argument, NULL, 'r' },
> + { "busid",  required_argument, NULL, 'b' },
> + { "device", no_argument,   NULL, 'd' },
> + { NULL, 0,  NULL, 0 }
> + };
> + char *host = NULL;
> + char *busid = NULL;
> + int opt;
> + int unbind = 1;
> + int ret = -1;
> +
> + for (;;) {
> + opt = getopt_long(argc, argv, "r:b:d", opts, NULL);
> +
> + if (opt == -1)
> + break;
> +
> + switch (opt) {
> + case 'r':
> + host = optarg;
> + break;
> + case 'b':
> + busid = optarg;
> + break;
> + case 'd':
> + driver = _driver;
> + unbind = 0;
> + break;
> + default:
> + goto err_out;
> + }
> + }
> +
> + if (!host || !busid)
> + goto err_out;
> +
> + ret = disconnect_device(host, busid, unbind);
> + goto out;

This looks a little bit weird... why you use goto out instead of just
return ret?

> +
> +err_out:
> + usbip_disconnect_usage();
> +out:
> + return ret;
> +}
> 

Best regards,
-- 
Krzysztof Opasiak
Samsung R Institute Poland
Samsung Electronics
--
To unsubscribe from this list: send the line 

[PATCH v13 04/10] usbip: exporting devices: new disconnect operation

[Nobuo Iwata]

Implementation of new disconnect 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 ... previous patch
 disconnect ... this patch

In device side node, this sends an un-export request, receives an 
un-export response and unbind corresponding device internally. The 
definition of the request and response are defined in original code: 
tools/usb/usbip/usbip_network.h but was not used. It's corresponding to 
NEW-4 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|   2 +-
 tools/usb/usbip/src/usbip.c|   6 +
 tools/usb/usbip/src/usbip.h|   2 +
 tools/usb/usbip/src/usbip_disconnect.c | 215 +
 4 files changed, 224 insertions(+), 1 deletion(-)

diff --git a/tools/usb/usbip/src/Makefile.am b/tools/usb/usbip/src/Makefile.am
index 0947476..42760c3 100644
--- a/tools/usb/usbip/src/Makefile.am
+++ b/tools/usb/usbip/src/Makefile.am
@@ -7,6 +7,6 @@ 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_connect.c
+usbip_connect.c usbip_disconnect.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 584d7d5..f0e9e06 100644
--- a/tools/usb/usbip/src/usbip.c
+++ b/tools/usb/usbip/src/usbip.c
@@ -83,6 +83,12 @@ static const struct command cmds[] = {
.usage = usbip_connect_usage
},
{
+   .name  = "disconnect",
+   .fn= usbip_disconnect,
+   .help  = "Disconnect a USB device from a remote computer",
+   .usage = usbip_disconnect_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 f365353..a8cbd16 100644
--- a/tools/usb/usbip/src/usbip.h
+++ b/tools/usb/usbip/src/usbip.h
@@ -32,6 +32,7 @@ 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[]);
+int usbip_disconnect(int argc, char *argv[]);
 
 void usbip_attach_usage(void);
 void usbip_detach_usage(void);
@@ -39,6 +40,7 @@ void usbip_list_usage(void);
 void usbip_bind_usage(void);
 void usbip_unbind_usage(void);
 void usbip_connect_usage(void);
+void usbip_disconnect_usage(void);
 
 int usbip_bind_device(char *busid);
 int usbip_unbind_device(char *busid);
diff --git a/tools/usb/usbip/src/usbip_disconnect.c 
b/tools/usb/usbip/src/usbip_disconnect.c
new file mode 100644
index 000..8155384
--- /dev/null
+++ b/tools/usb/usbip/src/usbip_disconnect.c
@@ -0,0 +1,215 @@
+/*
+ * 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 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program. If not, see .
+ */
+
+#include 
+
+#include 
+#include 
+#include 
+#include 
+
+#include 
+#include 
+