On 11/22/2016 07:48 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.
>
> 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 |
> +------+ +------------------+
> 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.iw...@fujixerox.co.jp>
> ---
> tools/usb/usbip/libsrc/vhci_driver.c | 99 ++++++++++++++++++++++++----
> tools/usb/usbip/libsrc/vhci_driver.h | 6 +-
> tools/usb/usbip/src/usbip_attach.c | 50 ++------------
> tools/usb/usbip/src/usbip_detach.c | 13 ++--
> 4 files changed, 100 insertions(+), 68 deletions(-)
>
> diff --git a/tools/usb/usbip/libsrc/vhci_driver.c
> b/tools/usb/usbip/libsrc/vhci_driver.c
> index ad92047..d2221c5 100644
> --- a/tools/usb/usbip/libsrc/vhci_driver.c
> +++ b/tools/usb/usbip/libsrc/vhci_driver.c
> @@ -1,5 +1,6 @@
> /*
> - * Copyright (C) 2005-2007 Takahiro Hirofuchi
> + * Copyright (C) 2015 Nobuo Iwata
> + * 2005-2007 Takahiro Hirofuchi
> */
>
> #include "usbip_common.h"
> @@ -7,6 +8,8 @@
> #include <limits.h>
> #include <netdb.h>
> #include <libudev.h>
> +#include <fcntl.h>
> +#include <errno.h>
> #include "sysfs_utils.h"
>
> #undef PROGNAME
> @@ -215,6 +218,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;
> +}
> +
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.
> /* ---------------------------------------------------------------------- */
>
> int usbip_vhci_driver_open(void)
> @@ -227,28 +249,21 @@ int usbip_vhci_driver_open(void)
>
> vhci_driver = calloc(1, sizeof(struct usbip_vhci_driver));
sizeof(*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 +292,8 @@ void usbip_vhci_driver_close(void)
>
> int usbip_vhci_refresh_device_list(void)
> {
> -
> + if (open_hc_device(OPEN_HC_MODE_REOPEN))
> + goto err;
1) one more enter
2) as above could you please share the reason of adding this?
> if (refresh_imported_device_list())
> goto err;
>
> @@ -409,3 +425,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);
sizeof(path)
> +
> + 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);
sizeof(buff)
generally if you are using snprintf you should check the returned value
to be sure you don't write some incomplete trash below.
> +
> + 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);
sizeof(path);
(...)
> static int import_device(int sockfd, struct usbip_usb_device *udev)
> {
> int rc;
> @@ -188,12 +145,13 @@ static int attach_device(char *host, char *busid)
> rhport = query_import_device(sockfd, busid);
> if (rhport < 0) {
> err("query");
> + close(sockfd);
> return -1;
> }
>
> close(sockfd);
why not just put close before if()?
>
> - rc = record_connection(host, usbip_port_string, busid, rhport);
> + rc = usbip_vhci_create_record(host, usbip_port_string, busid, rhport);
> if (rc < 0) {
> err("record connection");
> return -1;
> diff --git a/tools/usb/usbip/src/usbip_detach.c
> b/tools/usb/usbip/src/usbip_detach.c
> index 9db9d21..21ab710 100644
> --- a/tools/usb/usbip/src/usbip_detach.c
> +++ b/tools/usb/usbip/src/usbip_detach.c
> @@ -1,5 +1,6 @@
> /*
> - * Copyright (C) 2011 matt mooney <m...@muteddisk.com>
> + * Copyright (C) 2015 Nobuo Iwata
> + * 2011 matt mooney <m...@muteddisk.com>
> * 2005-2007 Takahiro Hirofuchi
> *
> * This program is free software: you can redistribute it and/or modify
> @@ -45,7 +46,6 @@ static int detach_port(char *port)
> {
> int ret;
> uint8_t portnum;
> - char path[PATH_MAX+1];
>
> unsigned int port_len = strlen(port);
>
> @@ -61,10 +61,7 @@ static int detach_port(char *port)
>
> /* remove the port state file */
>
> - snprintf(path, PATH_MAX, VHCI_STATE_PATH"/port%d", portnum);
> -
> - remove(path);
> - rmdir(VHCI_STATE_PATH);
> + usbip_vhci_delete_record(portnum);
>
> ret = usbip_vhci_driver_open();
> if (ret < 0) {
> @@ -73,8 +70,10 @@ static int detach_port(char *port)
> }
>
> ret = usbip_vhci_detach_device(portnum);
> - if (ret < 0)
> + if (ret < 0) {
> + usbip_vhci_driver_close();
> return -1;
> + }
>
> usbip_vhci_driver_close();
>
Why not just close driver before if()?
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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
