On 12/26/2016 08:08 AM, Nobuo Iwata wrote:
> 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?
> /* ---------------------------------------------------------------------- */
>
> 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
> +
> + 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
> +
> + 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?
> }
> } 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?
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
