On 11/22/2016 07:48 AM, Nobuo Iwata wrote:
> This patch is the new application(vhci)-side daemon specific code.
>
> The daemons are consisting three files.
> usbip.c : common code.
> usbip_dev.c: device(stub)-side specific code.
> usbip_app.c: application(vhci)-side specific code - this patch.
>
> Here, device-side daemon is EXISTING-1 and application-side daemon is
> NEW-1 in figure 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 --->
>
> Signed-off-by: Nobuo Iwata <nobuo.iwata@xxxxxxxxxxxxxxx>
> ---
> tools/usb/usbip/libsrc/vhci_driver.c | 19 +++
> tools/usb/usbip/libsrc/vhci_driver.h | 1 +
> tools/usb/usbip/src/Makefile.am | 7 +-
> tools/usb/usbip/src/usbipd.c | 12 +-
> tools/usb/usbip/src/usbipd_app.c | 242 +++++++++++++++++++++++++++
> 5 files changed, 279 insertions(+), 2 deletions(-)
>
> diff --git a/tools/usb/usbip/libsrc/vhci_driver.c b/tools/usb/usbip/libsrc/vhci_driver.c
> index d2221c5..3fe92ff 100644
> --- a/tools/usb/usbip/libsrc/vhci_driver.c
> +++ b/tools/usb/usbip/libsrc/vhci_driver.c
> @@ -314,6 +314,25 @@ int usbip_vhci_get_free_port(void)
> return -1;
> }
>
> +struct usbip_imported_device *usbip_vhci_find_device(char *host, char *busid)
> +{
> + int ret;
> + char rhost[NI_MAXHOST] = "unknown host";
> + char rserv[NI_MAXSERV] = "unknown port";
> + char rbusid[SYSFS_BUS_ID_SIZE];
> +
> + for (int i = 0; i < vhci_driver->nports; i++) {
> + ret = read_record(vhci_driver->idev[i].port, rhost, NI_MAXHOST,
> + rserv, NI_MAXSERV, rbusid);
sizeof(rhist), sizeof(rserv)
BTW.
The read_record() function is really weird and probably it should be
also refactored. We pass 3 arrays but pass size only for 2 of them and
size of third argument is hard coded inside this function:(
> + if (!ret &&
> + !strncmp(host, rhost, NI_MAXHOST) &&
> + !strncmp(busid, rbusid, SYSFS_BUS_ID_SIZE)) {
> + return vhci_driver->idev + i;
> + }
Is there any reason why you don't check port here?
I also thing that it's better to ensure that strings are \0 terminated
rather than using strncmp all the time.
> + }
> + return NULL;
> +}
> +
(...)
> +
> +static int import_device(int sockfd, struct usbip_usb_device *udev)
> +{
> + int rc;
> + int port;
> +
> + dbg("Sockfd:%d", sockfd);
> + port = usbip_vhci_get_free_port();
> + if (port < 0) {
> + err("no free port");
> + return -1;
> + }
> +
> + dump_usb_device(udev);
> + rc = usbip_vhci_attach_device(port, sockfd, udev->busnum,
> + udev->devnum, udev->speed);
> + if (rc < 0) {
> + err("import device");
> + return -1;
> + }
> +
> + return port;
> +}
> +
> +static int recv_request_export(int sockfd, char *host, char *port)
> +{
> + struct op_export_request req;
> + struct op_export_reply reply;
> + int rhport = 0;
> + int error = 0;
> + int rc;
> +
> + memset(&req, 0, sizeof(req));
> + memset(&reply, 0, sizeof(reply));
As before. You don't need to memset() reply.
> +
> + rc = usbip_net_recv(sockfd, &req, sizeof(req));
> + if (rc < 0) {
> + dbg("usbip_net_recv failed: export request");
> + return -1;
> + }
> + PACK_OP_EXPORT_REQUEST(0, &req);
> +
> + rhport = import_device(sockfd, &req.udev);
> + if (rhport < 0) {
> + dbg("export request busid %s: failed", req.udev.busid);
> + error = 1;
> + }
> +
> + rc = usbip_net_send_op_common(sockfd, OP_REP_EXPORT,
> + (!error ? ST_OK : ST_NA));
> + if (rc < 0) {
> + dbg("usbip_net_send_op_common failed: %#0x", OP_REP_EXPORT);
> + return -1;
> + }
> +
> + if (!error)
> + reply.returncode = 0;
> + else
> + reply.returncode = -1;
As I wrote in first patch. This may end up in compiler warning
> +
> + PACK_OP_EXPORT_REPLY(0, &rep);
> +
> + rc = usbip_net_send(sockfd, &reply, sizeof(reply));
> + if (rc < 0) {
> + dbg("usbip_net_send failed: export reply");
> + return -1;
> + }
> +
> + rc = usbip_vhci_create_record(host, port, req.udev.busid, rhport);
This call should be moved to import_device() as we may endup with
inconsistent_state() if there is a comminication error.
> + if (rc < 0) {
> + err("record connection");
> + return -1;
> + }
> +
> + dbg("export request busid %s: complete", req.udev.busid);
> +
> + return 0;
> +}
> +
> +static int unimport_device(char *host, struct usbip_usb_device *udev)
> +{
> + int rc;
> + struct usbip_imported_device *idev;
> +
> + idev = usbip_vhci_find_device(host, udev->busid);
> + if (idev == NULL) {
> + err("no imported port %s %s", host, udev->busid);
> + return -1;
> + }
> +
> + rc = usbip_vhci_detach_device(idev->port);
> + if (rc < 0) {
> + err("no imported port %d %s %s", idev->port, host, udev->busid);
> + return -1;
> + }
> + return idev->port;
> +}
> +
> +static int recv_request_unexport(int sockfd, char *host)
> +{
> + struct op_unexport_request req;
> + struct op_unexport_reply reply;
> + int rhport = 0;
> + int error = 0;
> + int rc;
> +
> + memset(&req, 0, sizeof(req));
> + memset(&reply, 0, sizeof(reply));
As before. You don't need to memset() reply.
> +
> + rc = usbip_net_recv(sockfd, &req, sizeof(req));
> + if (rc < 0) {
> + dbg("usbip_net_recv failed: unexport request");
> + return -1;
> + }
> + PACK_OP_UNEXPORT_REQUEST(0, &req);
> +
> + rhport = unimport_device(host, &req.udev);
> + if (rhport < 0)
> + error = 1;
> +
> + rc = usbip_net_send_op_common(sockfd, OP_REP_UNEXPORT,
> + (!error ? ST_OK : ST_NA));
> + if (rc < 0) {
> + dbg("usbip_net_send_op_common failed: %#0x", OP_REP_UNEXPORT);
> + return -1;
> + }
> +
> + if (!error) {
> + reply.returncode = 0;
> + } else {
> + reply.returncode = -1;
As I wrote in first patch. This may end up in compiler warning
> + dbg("unexport request busid %s: failed", req.udev.busid);
> + return -1;
> + }
> + PACK_OP_UNEXPORT_REPLY(0, &rep);
> +
> + rc = usbip_net_send(sockfd, &reply, sizeof(reply));
> + if (rc < 0) {
> + dbg("usbip_net_send failed: unexport reply");
> + return -1;
> + }
> +
> + usbip_vhci_delete_record(rhport);
I think that this function should be called from unimport_device()
Now this function can be ommited if there is some communication error
and we may endup in inconsistent state.
> +
> + dbg("unexport request busid %s: complete", req.udev.busid);
> +
> + return 0;
> +}
> +
> +int usbip_recv_pdu(int connfd, char *host, char *port)
> +{
> + uint16_t code = OP_UNSPEC;
> + int ret;
> +
> + ret = usbip_net_recv_op_common(connfd, &code);
> + if (ret < 0) {
> + dbg("could not receive opcode: %#0x", code);
> + return -1;
> + }
> +
> + ret = usbip_vhci_refresh_device_list();
> + if (ret < 0) {
> + dbg("could not refresh device list: %d", ret);
> + return -1;
> + }
> +
> + info("received request: %#0x(%d)", code, connfd);
> + switch (code) {
> + case OP_REQ_EXPORT:
> + ret = recv_request_export(connfd, host, port);
> + break;
> + case OP_REQ_UNEXPORT:
> + ret = recv_request_unexport(connfd, host);
> + break;
> + default:
> + err("received an unknown opcode: %#0x", code);
> + ret = -1;
> + }
> +
> + if (ret == 0)
> + info("request %#0x(%s:%s): complete", code, host, port);
> + else
> + info("request %#0x(%s:%s): failed", code, host, port);
> +
> + return ret;
> +}
>
This one is a copy-paste from usbip_recv_pdu() in usbipd_dev.c with only
values changed in switch. How about sharing this code and add only
callback which will be used based on received op_code?
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
