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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hello,

> > Implementation of new connect operation. This is linked as a part of
> > usbip command. With this patch, usbip command has following
> operations.
> >
> >  bind
> >  unbind
> >  list (local/remote)
> >  attach
> >  detach
> >  port
> >  connect ... this patch
> 
> Don't include existing commands. Just list the new one.

OK.

> Why do you call this connect. Isn't it really export
> - connect isn't accurate.
> Call it export/unexport.

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 will change it if there's user friendly, well-known and fit to
existing operations.

> > 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 |
> >          +------+
> +------------------+
> >          (server)                               (client)
> >  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(stub)-side
> >          +------+
> +------------------+
> >  device--+ STUB |                               | application/VHCI |
> >          +------+
> +------------------+
> >          (client)                               (server)
> 
> Make the left side server and right side client. I think you might be
> using server and client network terminology. I would like to see server
> as the system that has the device physically attached to.
> 
> I still want to see server as the one that is connected to the device.
> It is just that in this new proposed model, server is exporting devices.

I know that existing USB/IP users are familiar with word 'server'.
It was consistent to client-server model.
In new operation, to call server the device side node will cause
 confusion because it is a client as client-server model.
So I think it's better to use 'device-side' and 'application-side' to
specify nodes. These word can intuitively denote the nodes.
In this version, I marked (server) and (client) but I will remove
them because it causes existing user's confusion.

> Also does usbip run here in user mode. Can any user run uspip and initiate
> export? If so, I don't think I can take this patch series. I don't like to
> see non root being able to export and/or make attached devices public.

New commands and daemon needs root privilege same as 'bind', 'unbind'
 and 'attach'.

They uses same sysfs interface to 'bind', 'unbind' and 'attach'.

> >                                             1) # usbipa ... start
> daemon
> 
> Did you mean uspipd or is uspipa a new daemon?
> Okay now I see that there is new daemon introduced in patch 7/10 -
> Why do you need to add a new daemon? Why not add it to usbipd instead?

By separating usbipa, administrators can select to use or not to use
new operation. If they want to use, run usbipa.

Also to simplify dependency.
  - usbipd depends stub and vudc.
  - usbipa depends vhci.

> I would like to see all of this information in README. Including this
> in every single patch is confusing.
> btw you are also missing doc changes for these new options you are
> adding.
> When you change README, please include the both server and client end
> commands just like README does now.

README and doc is changed by 9/10 change to documentation.

> >  = = =
> >  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 <nobuo.iwata@xxxxxxxxxxxxxxx>
> > ---
> >  tools/usb/usbip/src/Makefile.am     |   3 +-
> >  tools/usb/usbip/src/usbip.c         |   7 +
> >  tools/usb/usbip/src/usbip.h         |   3 +
> >  tools/usb/usbip/src/usbip_connect.c | 212
> ++++++++++++++++++++++++++++
> >  4 files changed, 224 insertions(+), 1 deletion(-)
> >
> > 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..ad2a259 100644
> > --- a/tools/usb/usbip/src/usbip.c
> > +++ b/tools/usb/usbip/src/usbip.c
> > @@ -4,6 +4,7 @@
> >   *
> >   * Copyright (C) 2011 matt mooney <mfm@xxxxxxxxxxxxx>
> >   *               2005-2007 Takahiro Hirofuchi
> > + * Copyright (C) 2015-2016 Nobuo Iwata
> <nobuo.iwata@xxxxxxxxxxxxxxx>
> >   *
> >   * 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
> > @@ -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..3c22c27 100644
> > --- a/tools/usb/usbip/src/usbip.h
> > +++ b/tools/usb/usbip/src/usbip.h
> > @@ -1,6 +1,7 @@
> >  /*
> >   * Copyright (C) 2011 matt mooney <mfm@xxxxxxxxxxxxx>
> >   *               2005-2007 Takahiro Hirofuchi
> > + * Copyright (C) 2015-2016 Nobuo Iwata
> <nobuo.iwata@xxxxxxxxxxxxxxx>
> >   *
> >   * 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
> > @@ -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 0000000..73668db2
> > --- /dev/null
> > +++ b/tools/usb/usbip/src/usbip_connect.c
> > @@ -0,0 +1,212 @@
> > +/*
> > + * Copyright (C) 2011 matt mooney <mfm@xxxxxxxxxxxxx>
> > + *               2005-2007 Takahiro Hirofuchi
> > + * Copyright (C) 2015-2016 Nobuo Iwata
> <nobuo.iwata@xxxxxxxxxxxxxxx>
> > + *
> > + * 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
> <http://www.gnu.org/licenses/>.
> > + */
> > +
> > +#include <sys/stat.h>
> > +
> > +#include <limits.h>
> > +#include <stdint.h>
> > +#include <stdio.h>
> > +#include <string.h>
> > +
> > +#include <getopt.h>
> > +#include <unistd.h>
> > +
> > +#include "usbip_host_driver.h"
> > +#include "usbip_host_common.h"
> > +#include "usbip_device_driver.h"
> > +#include "usbip_common.h"
> > +#include "usbip_network.h"
> > +#include "usbip.h"
> > +
> > +static struct usbip_host_driver *driver = &host_driver;
> > +
> > +static const char usbip_connect_usage_string[] =
> > +	"usbip connect <args>\n"
> > +	"    -r, --remote=<host>    Address of a remote computer\n"
> > +	"    -b, --busid=<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;
> > +	uint16_t code = OP_REP_EXPORT;
> > +
> > +	/* send a request */
> > +	rc = usbip_net_send_op_common(sockfd, OP_REQ_EXPORT, 0);
> > +	if (rc < 0) {
> > +		err("send op_common");
> > +		return -1;
> > +	}
> > +
> > +	memset(&request, 0, sizeof(request));
> > +	memcpy(&request.udev, udev, sizeof(request.udev));
> > +
> > +	PACK_OP_EXPORT_REQUEST(0, &request);
> > +
> > +	rc = usbip_net_send(sockfd, (void *) &request, sizeof(request));
> > +	if (rc < 0) {
> > +		err("send op_export_request");
> > +		return -1;
> > +	}
> > +
> > +	/* receive a reply */
> > +	rc = usbip_net_recv_op_common(sockfd, &code);
> > +	if (rc < 0) {
> > +		err("recv op_common");
> > +		return -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");
> > +		goto err_out;
> > +	}
> > +
> > +	rc = usbip_refresh_device_list(driver);
> > +	if (rc < 0) {
> > +		err("could not refresh device list");
> > +		goto err_driver_close;
> > +	}
> > +
> > +	edev = usbip_get_device(driver, busid);
> > +	if (edev == NULL) {
> > +		err("find device");
> > +		goto err_driver_close;
> > +	}
> > +
> > +	rc = send_export_device(sockfd, &edev->udev);
> > +	if (rc < 0) {
> > +		err("send export");
> > +		goto err_driver_close;
> > +	}
> > +
> > +	rc = usbip_export_device(edev, sockfd);
> > +	if (rc < 0) {
> > +		err("export device");
> > +		goto err_driver_close;
> > +	}
> > +
> > +	usbip_driver_close(driver);
> > +
> > +	return 0;
> > +
> > +err_driver_close:
> > +	usbip_driver_close(driver);
> > +err_out:
> > +	return -1;
> > +}
> > +
> > +static int connect_device(char *host, char *busid, int bind)
> > +{
> > +	int sockfd;
> > +	int rc;
> > +
> > +	if (bind) {
> > +		rc = usbip_bind_device(busid);
> > +		if (rc) {
> > +			err("bind");
> > +			goto err_out;
> > +		}
> > +	}
> > +
> > +	sockfd = usbip_net_tcp_connect(host, usbip_port_string);
> > +	if (sockfd < 0) {
> > +		err("tcp connect");
> > +		goto err_unbind_device;
> > +	}
> > +
> > +	rc = export_device(busid, sockfd);
> > +	if (rc < 0) {
> > +		err("export");
> > +		goto err_close_conn;
> > +	}
> > +
> > +	close(sockfd);
> > +
> > +	return 0;
> > +err_close_conn:
> > +	close(sockfd);
> > +err_unbind_device:
> > +	if (bind)
> > +		usbip_unbind_device(busid);
> > +err_out:
> > +	return -1;
> > +}
> > +
> > +int usbip_connect(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 bind = 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 = &device_driver;
> > +			bind = 0;
> > +			break;
> > +		default:
> > +			goto err_out;
> > +		}
> > +	}
> > +
> > +	if (!host || !busid)
> > +		goto err_out;
> > +
> > +	ret = connect_device(host, busid, bind);
> > +	goto out;
> > +
> > +err_out:
> > +	usbip_connect_usage();
> > +out:
> > +	return ret;
> > +}
> >

Sorry for my late response,

Nobuo Iwata
//
--
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




[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux