Re: [PATCH v2 4/6] usb: usbip tool: Fix refresh_imported_device_list()
Hi Khan,
On Tue, Apr 04, 2017 at 11:14:33AM -0600, Shuah Khan wrote:
> On 03/30/2017 06:28 PM, Yuyang Du wrote:
> > The commit 0775a9cbc694e8c7 ("usbip: vhci extension: modifications
> > to vhci driver") introduced multiple controllers, but the status
> > of the ports are only extracted from the first status file, fix it.
>
> Are sure this change is needed? What bug are you fixing? This change
> will impact every single code path that calls usbip_vhci_driver_open()
>
> It might be sufficient to look at just the first file status in many of
> these cases?
Yes, I'm sure. The 1st status file only has the 1st controller's ports,
but we must have the status of all the ports of all the controllers.
> >
> > Signed-off-by: Yuyang Du
> > ---
> > tools/usb/usbip/libsrc/vhci_driver.c | 27 +--
> > 1 file changed, 21 insertions(+), 6 deletions(-)
> >
> > diff --git a/tools/usb/usbip/libsrc/vhci_driver.c
> > b/tools/usb/usbip/libsrc/vhci_driver.c
> > index ccecd47..630b139 100644
> > --- a/tools/usb/usbip/libsrc/vhci_driver.c
> > +++ b/tools/usb/usbip/libsrc/vhci_driver.c
> > @@ -108,18 +108,33 @@ static int parse_status(const char *value)
> > return 0;
> > }
> >
> > +#define MAX_STATUS_NAME 16
> > +
> > static int refresh_imported_device_list(void)
> > {
> > const char *attr_status;
> > + char status[MAX_STATUS_NAME+1] = "status";
> > + int i, ret;
> >
> > - attr_status = udev_device_get_sysattr_value(vhci_driver->hc_device,
> > - "status");
> > - if (!attr_status) {
> > - err("udev_device_get_sysattr_value failed");
> > - return -1;
> > + for (i = 0; i < vhci_driver->ncontrollers; i++) {
> > + if (i > 0)
> > + snprintf(status, sizeof(status), "status.%d", i);
> > +
> > + attr_status =
> > udev_device_get_sysattr_value(vhci_driver->hc_device,
> > + status);
> > + if (!attr_status) {
> > + err("udev_device_get_sysattr_value failed");
> > + return -1;
> > + }
> > +
> > + dbg("controller %d", i);
> > +
> > + ret = parse_status(attr_status);
> > + if (ret != 0)
> > + return ret;
>
> usbip_vhci_driver_open() will fail even if one of these fails? Is that
> what you would want?
Ditto.
Thanks,
Yuyang
--
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
Re: [PATCH v2 4/6] usb: usbip tool: Fix refresh_imported_device_list()
On 03/30/2017 06:28 PM, Yuyang Du wrote:
> The commit 0775a9cbc694e8c7 ("usbip: vhci extension: modifications
> to vhci driver") introduced multiple controllers, but the status
> of the ports are only extracted from the first status file, fix it.
Are sure this change is needed? What bug are you fixing? This change
will impact every single code path that calls usbip_vhci_driver_open()
It might be sufficient to look at just the first file status in many of
these cases?
>
> Signed-off-by: Yuyang Du
> ---
> tools/usb/usbip/libsrc/vhci_driver.c | 27 +--
> 1 file changed, 21 insertions(+), 6 deletions(-)
>
> diff --git a/tools/usb/usbip/libsrc/vhci_driver.c
> b/tools/usb/usbip/libsrc/vhci_driver.c
> index ccecd47..630b139 100644
> --- a/tools/usb/usbip/libsrc/vhci_driver.c
> +++ b/tools/usb/usbip/libsrc/vhci_driver.c
> @@ -108,18 +108,33 @@ static int parse_status(const char *value)
> return 0;
> }
>
> +#define MAX_STATUS_NAME 16
> +
> static int refresh_imported_device_list(void)
> {
> const char *attr_status;
> + char status[MAX_STATUS_NAME+1] = "status";
> + int i, ret;
>
> - attr_status = udev_device_get_sysattr_value(vhci_driver->hc_device,
> -"status");
> - if (!attr_status) {
> - err("udev_device_get_sysattr_value failed");
> - return -1;
> + for (i = 0; i < vhci_driver->ncontrollers; i++) {
> + if (i > 0)
> + snprintf(status, sizeof(status), "status.%d", i);
> +
> + attr_status =
> udev_device_get_sysattr_value(vhci_driver->hc_device,
> + status);
> + if (!attr_status) {
> + err("udev_device_get_sysattr_value failed");
> + return -1;
> + }
> +
> + dbg("controller %d", i);
> +
> + ret = parse_status(attr_status);
> + if (ret != 0)
> + return ret;
usbip_vhci_driver_open() will fail even if one of these fails? Is that
what you would want?
thanks,
-- Shuah
> }
>
> - return parse_status(attr_status);
> + return 0;
> }
>
> static int get_nports(void)
>
--
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
Re: [PATCH v2 4/6] usb: usbip tool: Fix refresh_imported_device_list()
On 03/31/2017 02:28 AM, Yuyang Du wrote:
The commit 0775a9cbc694e8c7 ("usbip: vhci extension: modifications
to vhci driver") introduced multiple controllers, but the status
of the ports are only extracted from the first status file, fix it.
Signed-off-by: Yuyang Du
---
tools/usb/usbip/libsrc/vhci_driver.c | 27 +--
1 file changed, 21 insertions(+), 6 deletions(-)
diff --git a/tools/usb/usbip/libsrc/vhci_driver.c
b/tools/usb/usbip/libsrc/vhci_driver.c
index ccecd47..630b139 100644
--- a/tools/usb/usbip/libsrc/vhci_driver.c
+++ b/tools/usb/usbip/libsrc/vhci_driver.c
@@ -108,18 +108,33 @@ static int parse_status(const char *value)
return 0;
}
+#define MAX_STATUS_NAME 16
+
static int refresh_imported_device_list(void)
{
const char *attr_status;
+ char status[MAX_STATUS_NAME+1] = "status";
+ int i, ret;
- attr_status = udev_device_get_sysattr_value(vhci_driver->hc_device,
- "status");
- if (!attr_status) {
- err("udev_device_get_sysattr_value failed");
- return -1;
+ for (i = 0; i < vhci_driver->ncontrollers; i++) {
+ if (i > 0)
+ snprintf(status, sizeof(status), "status.%d", i);
+
+ attr_status =
udev_device_get_sysattr_value(vhci_driver->hc_device,
+ status);
+ if (!attr_status) {
+ err("udev_device_get_sysattr_value failed");
+ return -1;
+ }
+
+ dbg("controller %d", i);
+
+ ret = parse_status(attr_status);
+ if (ret != 0)
+ return ret;
}
- return parse_status(attr_status);
+ return 0;
}
I decided to not urge here with OO programming introduction so just
ignore my previous comment and add:
Reviewed-by: Krzysztof Opasiak
Cheers,
--
Krzysztof Opasiak
Samsung R 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
[PATCH v2 4/6] usb: usbip tool: Fix refresh_imported_device_list()
The commit 0775a9cbc694e8c7 ("usbip: vhci extension: modifications
to vhci driver") introduced multiple controllers, but the status
of the ports are only extracted from the first status file, fix it.
Signed-off-by: Yuyang Du
---
tools/usb/usbip/libsrc/vhci_driver.c | 27 +--
1 file changed, 21 insertions(+), 6 deletions(-)
diff --git a/tools/usb/usbip/libsrc/vhci_driver.c
b/tools/usb/usbip/libsrc/vhci_driver.c
index ccecd47..630b139 100644
--- a/tools/usb/usbip/libsrc/vhci_driver.c
+++ b/tools/usb/usbip/libsrc/vhci_driver.c
@@ -108,18 +108,33 @@ static int parse_status(const char *value)
return 0;
}
+#define MAX_STATUS_NAME 16
+
static int refresh_imported_device_list(void)
{
const char *attr_status;
+ char status[MAX_STATUS_NAME+1] = "status";
+ int i, ret;
- attr_status = udev_device_get_sysattr_value(vhci_driver->hc_device,
- "status");
- if (!attr_status) {
- err("udev_device_get_sysattr_value failed");
- return -1;
+ for (i = 0; i < vhci_driver->ncontrollers; i++) {
+ if (i > 0)
+ snprintf(status, sizeof(status), "status.%d", i);
+
+ attr_status =
udev_device_get_sysattr_value(vhci_driver->hc_device,
+ status);
+ if (!attr_status) {
+ err("udev_device_get_sysattr_value failed");
+ return -1;
+ }
+
+ dbg("controller %d", i);
+
+ ret = parse_status(attr_status);
+ if (ret != 0)
+ return ret;
}
- return parse_status(attr_status);
+ return 0;
}
static int get_nports(void)
--
2.7.4
--
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
