Hello,
> > Refactoring to attach and detach operation to reuse common portion to
> > application(vhci)-side daemon.
> >
> > The new application(vhci)-side daemon executes same procedures as
> > attach and detach. Most of common code to access vhci has been
> > implemented in VHCI userspace wrapper(libsrc/vhci_driver.c) but left in
> > attach and detach. With this patch, an accessor of vhci driver and
> > tracking of vhci connections in attach and detach are moved to the VHCI
> > userspace wrapper.
> >
> > Functions to create and delete port status file also moved to the
> > wrapper. Timing to execute these function are corrected to keep
> > consistent with attached status of vhci.
> >
> > Here, attach, detach and application(vhci)-side daemon is EXISTING-5,
> > EXISTING-6 and NEW-1 respectively in diagram below.
> >
> > 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)
> > 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 <nobuo.iwata@xxxxxxxxxxxxxxx>
> > ---
> > tools/usb/usbip/libsrc/vhci_driver.c | 96
> +++++++++++++++++++++++----
> > tools/usb/usbip/libsrc/vhci_driver.h | 3 +
> > tools/usb/usbip/src/usbip_attach.c | 99
> +++++++++-------------------
> > tools/usb/usbip/src/usbip_detach.c | 17 ++---
> > 4 files changed, 124 insertions(+), 91 deletions(-)
> >
> > diff --git a/tools/usb/usbip/libsrc/vhci_driver.c
> b/tools/usb/usbip/libsrc/vhci_driver.c
> > index ad92047..5843f43 100644
> > --- a/tools/usb/usbip/libsrc/vhci_driver.c
> > +++ b/tools/usb/usbip/libsrc/vhci_driver.c
> > @@ -7,6 +7,8 @@
> > #include <limits.h>
> > #include <netdb.h>
> > #include <libudev.h>
> > +#include <fcntl.h>
> > +#include <errno.h>
> > #include "sysfs_utils.h"
> >
> > #undef PROGNAME
> > @@ -215,6 +217,25 @@ static int read_record(int rhport, char *host,
> unsigned long host_len,
> > return 0;
> > }
> >
> > +#define OPEN_HC_MODE_FIRST 0
> > +#define OPEN_HC_MODE_REOPEN 1
> > +
> > +static int open_hc_device(int mode)
> > +{
> > + if (mode == OPEN_HC_MODE_REOPEN)
> > + udev_device_unref(vhci_driver->hc_device);
> > +
> > + vhci_driver->hc_device =
> > +
> udev_device_new_from_subsystem_sysname(udev_context,
> > +
> USBIP_VHCI_BUS_TYPE,
> > +
> USBIP_VHCI_DRV_NAME);
> > + if (!vhci_driver->hc_device) {
> > + err("udev_device_new_from_subsystem_sysname
> failed");
> > + return -1;
> > + }
> > + return 0;
> > +}
> > +
>
> Please let mi cite my previous email:
>
> <<<<
> Could you please elaborate why this function takes an argument and why
> you are reopening vhci device while refreshing device list? there is
> nothing about this in commit msg.
> >>>>
>
> could you please answer?
Sorry, I missed the comment.
For new usbipa daemon, vhci_driver->hc_device life cycle is here,
usbip_vhci_driver_open() : new
usbip_vhci_refresh_device_list() for each request: unref, new
usbip_vhci_driver_close() : unref
In vanilla, usbip_vhci_refresh_device_list() has been defined but not
used. And it doesn't work because it doesn't have unref, new.
I refactored unref+new as open_hc_device(do_unref_flag) but I didn't
create close_hc_device().
With your comment, I think it's better to make open_hc_device() as new,
close_hc_device() as unref and usbip_vhci_refresh_device_list() calls
close_hc_device() and open_hc_device().
> > /*
> ----------------------------------------------------------------------
> */
> >
> > int usbip_vhci_driver_open(void)
> > @@ -227,28 +248,21 @@ int usbip_vhci_driver_open(void)
> >
> > vhci_driver = calloc(1, sizeof(struct usbip_vhci_driver));
> >
> > - /* will be freed in usbip_driver_close() */
> > - vhci_driver->hc_device =
> > -
> udev_device_new_from_subsystem_sysname(udev_context,
> > -
> USBIP_VHCI_BUS_TYPE,
> > -
> USBIP_VHCI_DRV_NAME);
> > - if (!vhci_driver->hc_device) {
> > - err("udev_device_new_from_subsystem_sysname
> failed");
> > - goto err;
> > - }
> > + if (open_hc_device(OPEN_HC_MODE_FIRST))
> > + goto err_free_driver;
> >
> > vhci_driver->nports = get_nports();
> >
> > dbg("available ports: %d", vhci_driver->nports);
> >
> > if (refresh_imported_device_list())
> > - goto err;
> > + goto err_unref_device;
> >
> > return 0;
> >
> > -err:
> > +err_unref_device:
> > udev_device_unref(vhci_driver->hc_device);
> > -
> > +err_free_driver:
> > if (vhci_driver)
> > free(vhci_driver);
> >
> > @@ -277,7 +291,8 @@ void usbip_vhci_driver_close(void)
> >
> > int usbip_vhci_refresh_device_list(void)
> > {
> > -
> > + if (open_hc_device(OPEN_HC_MODE_REOPEN))
> > + goto err;
>
> empty line
>
> > if (refresh_imported_device_list())
> > goto err;
> >
> > @@ -409,3 +424,58 @@ int usbip_vhci_imported_device_dump(struct
> usbip_imported_device *idev)
> >
> > return 0;
> > }
> > +
> > +#define MAX_BUFF 100
> > +int usbip_vhci_create_record(char *host, char *port, char *busid, int
> rhport)
> > +{
> > + int fd;
> > + char path[PATH_MAX+1];
> > + char buff[MAX_BUFF+1];
> > + int ret;
> > +
> > + ret = mkdir(VHCI_STATE_PATH, 0700);
> > + if (ret < 0) {
> > + /* if VHCI_STATE_PATH exists, then it better be a
> directory */
> > + if (errno == EEXIST) {
> > + struct stat s;
> > +
> > + ret = stat(VHCI_STATE_PATH, &s);
> > + if (ret < 0)
> > + return -1;
> > + if (!(s.st_mode & S_IFDIR))
> > + return -1;
> > + } else
> > + return -1;
> > + }
> > +
> > + snprintf(path, PATH_MAX, VHCI_STATE_PATH"/port%d", rhport);
> > +
> > + fd = open(path, O_WRONLY|O_CREAT|O_TRUNC, 0700);
> > + if (fd < 0)
> > + return -1;
> > +
> > + snprintf(buff, MAX_BUFF, "%s %s %s\n",
> > + host, port, busid);
>
> if you use snprintf() it would be nice to check the returned value
It's moved from usbip_attach.c: record_connection() but I will fix.
> > +
> > + ret = write(fd, buff, strlen(buff));
> > + if (ret != (ssize_t) strlen(buff)) {
> > + close(fd);
> > + return -1;
> > + }
> > +
> > + close(fd);
> > +
> > + return 0;
> > +}
> > +
> > +int usbip_vhci_delete_record(int rhport)
> > +{
> > + char path[PATH_MAX+1];
> > +
> > + snprintf(path, PATH_MAX, VHCI_STATE_PATH"/port%d", rhport);
> > +
> > + remove(path);
> > + rmdir(VHCI_STATE_PATH);
>
> it would be nice to add a comment here that you don't check the return
> value because it will succeed only when all files had been removed
OK. Thanks!
> > +
> > + return 0;
> > +}
> > diff --git a/tools/usb/usbip/libsrc/vhci_driver.h
> b/tools/usb/usbip/libsrc/vhci_driver.h
> > index fa2316c..c85988c 100644
> > --- a/tools/usb/usbip/libsrc/vhci_driver.h
> > +++ b/tools/usb/usbip/libsrc/vhci_driver.h
> > @@ -54,6 +54,9 @@ int usbip_vhci_attach_device(uint8_t port, int
> sockfd, uint8_t busnum,
> >
> > int usbip_vhci_detach_device(uint8_t port);
> >
> > +int usbip_vhci_create_record(char *host, char *port, char *busid, int
> rhport);
> > +int usbip_vhci_delete_record(int rhport);
> > +
> > int usbip_vhci_imported_device_dump(struct usbip_imported_device
> *idev);
> >
> > #endif /* __VHCI_DRIVER_H */
> > diff --git a/tools/usb/usbip/src/usbip_attach.c
> b/tools/usb/usbip/src/usbip_attach.c
> > index 695b2e5..17a84ea 100644
> > --- a/tools/usb/usbip/src/usbip_attach.c
> > +++ b/tools/usb/usbip/src/usbip_attach.c
> > @@ -4,6 +4,7 @@
> > * Copyright (C) 2015-2016 Samsung Electronics
> > * Igor Kotrasinski <i.kotrasinsk@xxxxxxxxxxx>
> > * Krzysztof Opasiak <k.opasiak@xxxxxxxxxxx>
> > + * 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
> > @@ -26,7 +27,6 @@
> > #include <stdio.h>
> > #include <string.h>
> >
> > -#include <fcntl.h>
> > #include <getopt.h>
> > #include <unistd.h>
> > #include <errno.h>
> > @@ -47,83 +47,53 @@ void usbip_attach_usage(void)
> > printf("usage: %s", usbip_attach_usage_string);
> > }
> >
> > -#define MAX_BUFF 100
> > -static int record_connection(char *host, char *port, char *busid, int
> rhport)
> > -{
> > - int fd;
> > - char path[PATH_MAX+1];
> > - char buff[MAX_BUFF+1];
> > - int ret;
> > -
> > - ret = mkdir(VHCI_STATE_PATH, 0700);
> > - if (ret < 0) {
> > - /* if VHCI_STATE_PATH exists, then it better be a
> directory */
> > - if (errno == EEXIST) {
> > - struct stat s;
> > -
> > - ret = stat(VHCI_STATE_PATH, &s);
> > - if (ret < 0)
> > - return -1;
> > - if (!(s.st_mode & S_IFDIR))
> > - return -1;
> > - } else
> > - return -1;
> > - }
> > -
> > - snprintf(path, PATH_MAX, VHCI_STATE_PATH"/port%d", rhport);
> > -
> > - fd = open(path, O_WRONLY|O_CREAT|O_TRUNC, S_IRWXU);
> > - if (fd < 0)
> > - return -1;
> > -
> > - snprintf(buff, MAX_BUFF, "%s %s %s\n",
> > - host, port, busid);
> > -
> > - ret = write(fd, buff, strlen(buff));
> > - if (ret != (ssize_t) strlen(buff)) {
> > - close(fd);
> > - return -1;
> > - }
> > -
> > - close(fd);
> > -
> > - return 0;
> > -}
> > -
> > -static int import_device(int sockfd, struct usbip_usb_device *udev)
> > +static int import_device(int sockfd, struct usbip_usb_device *udev,
> > + char *host, char *port, char *busid)
> > {
> > int rc;
> > - int port;
> > + int rhport;
> >
> > rc = usbip_vhci_driver_open();
> > if (rc < 0) {
> > err("open vhci_driver");
> > - return -1;
> > + goto err_out;
> > }
> >
> > do {
> > - port = usbip_vhci_get_free_port();
> > - if (port < 0) {
> > + rhport = usbip_vhci_get_free_port();
> > + if (rhport < 0) {
> > err("no free port");
> > - usbip_vhci_driver_close();
> > - return -1;
> > + goto err_driver_close;
> > }
> >
> > - rc = usbip_vhci_attach_device(port, sockfd,
> udev->busnum,
> > + rc = usbip_vhci_attach_device(rhport, sockfd,
> udev->busnum,
> > udev->devnum,
> udev->speed);
> > if (rc < 0 && errno != EBUSY) {
> > err("import device");
> > - usbip_vhci_driver_close();
> > + goto err_driver_close;
> > return -1;
>
> Am I missing sth or this return here is unreachable?
Sorry. I will fix 'usbip: auto retry for concurrent attach' patch.
> > }
> > } while (rc < 0);
> >
> > + rc = usbip_vhci_create_record(host, port, busid, rhport);
> > + if (rc < 0) {
> > + err("record connection");
> > + goto err_detach_device;
> > + }
> > +
> > usbip_vhci_driver_close();
> >
> > - return port;
> > + return 0;
> > +
> > +err_detach_device:
> > + usbip_vhci_detach_device(rhport);
> > +err_driver_close:
> > + usbip_vhci_driver_close();
> > +err_out:
> > + return -1;
> > }
> >
> > -static int query_import_device(int sockfd, char *busid)
> > +static int query_import_device(int sockfd, char *host, char *port, char
> *busid)
> > {
> > int rc;
> > struct op_import_request request;
> > @@ -171,15 +141,13 @@ static int query_import_device(int sockfd, char
> *busid)
> > return -1;
> > }
> >
> > - /* import a device */
> > - return import_device(sockfd, &reply.udev);
> > + return import_device(sockfd, &reply.udev, host, port, busid);
> > }
> >
> > -static int attach_device(char *host, char *busid)
> > +static int attach_device(char *host, char *port, char *busid)
> > {
> > int sockfd;
> > int rc;
> > - int rhport;
> >
> > sockfd = usbip_net_tcp_connect(host, usbip_port_string);
> > if (sockfd < 0) {
> > @@ -187,20 +155,15 @@ static int attach_device(char *host, char
> *busid)
> > return -1;
> > }
> >
> > - rhport = query_import_device(sockfd, busid);
> > - if (rhport < 0) {
> > + rc = query_import_device(sockfd, host, port, busid);
> > + if (rc < 0) {
> > err("query");
> > + close(sockfd);
> > return -1;
> > }
> >
> > close(sockfd);
>
> Once again the same pater:
> ret = func();
> if (cond(ret)) {
> do_stuff();
> do_other_stuff();
> }
>
> do_stuff();
>
> Why not just:
>
> ret = fun();
> do_stuff();
> if (ret)
> do_other_stuff();
>
> >
> > - rc = record_connection(host, usbip_port_string, busid, rhport);
> > - if (rc < 0) {
> > - err("record connection");
> > - return -1;
> > - }
> > -
> > return 0;
> > }
>
> Why didn't you put this in a separate commit as we talked during
> previous revision?
Very sorry. I checked my sent e-mails and I missed to respond 06/10.
But I couldn't find about separation about this patch.
http://www.mail-archive.com/linux-usb@xxxxxxxxxxxxxxx/msg84077.html
We talked in 8/10 change to usbip_list.c.
http://www.mail-archive.com/linux-usb@xxxxxxxxxxxxxxx/msg84246.html
>> In addition I think that this patch should be send separately before
>> this whole series.
> After this set is re-arranged. I will remind it.
> I may include this patch because (it can be said that) this caused by
> introducing export request.
For 06/10, I will separate improvement to original code.
Modification for this patch set will be remained in this set.
For 08/10, it will be separated into two patches out of this series,
1) move close().
2) fix correction to wording inconsistency around import and export.
> Best regards,
> --
> Krzysztof Opasiak
> Samsung R&D Institute Poland
> Samsung Electronics
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
