On 01/13/2017 04:46 PM, Shuah Khan wrote:
> On 01/13/2017 04:55 AM, Krzysztof Opasiak wrote:
>>
>>
>> On 01/12/2017 10:14 PM, Shuah Khan wrote:
>>> On 12/26/2016 12:08 AM, Nobuo Iwata wrote:
>>>> Modifications to host driver wrapper and its related operations (i.e.
>>>> bind/unbind).
>>>
>>> Way too many changes in this one patch.
>>>
>>>>
>>>> usbip_get_device() method was not used. The implementation of the
>>>> method searches a bound devices list by list index. The position in the
>>>> list is meaningless for any operations so it is assumed not to be used
>>>> in future.
>>>>
>>>> For this patch set, a method to find a bound device in the bound
>>>> devices list by bus-id is needed. usbip_get_device() is re-implemented
>>>> as a function to find a bound device by bus-id.
>>>
>>> This is not an accurate description. You are really changing look ups using
>>> bus-id instead bus-num and whether usbip_get_device() is used or not is
>>> irrelevant.
>>>
>>> Please make changes to find bound device by bus-id in a separate patch.
>>> You will have to change usbip_get_device() anyway because you are changing
>>> .get_device() interface. Include exporting usbip_get_debice() in a separate
>>> patch.
>>>
>>>>
>>>> bind and unbind function are exported to reuse by new connect and
>>>> disconnect operation. Here, connect and disconnect is NEW-3 and NEW-4
>>>> respactively in diagram below.
>>>
>>> Please don't include exporting bind_device() and unbind_device() and
>>> changing their names in this patch. Send a separate patch for that.
>>>
>>> It makes sense to move the functions you are exporting bind_device(),
>>> unbind_device(), and usbip_get_device() to libsrc - usbip_common.c
>>> might be a good choice.
>>>
>>> The following isn't part of this patch, hence shouldn't be included in
>>> the change log.
>>>
>>>>
>>>> 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/usbip_host_common.c | 6 ++----
>>>> tools/usb/usbip/libsrc/usbip_host_common.h | 8 ++++----
>>>> tools/usb/usbip/src/usbip.h | 3 +++
>>>> tools/usb/usbip/src/usbip_bind.c | 4 ++--
>>>> tools/usb/usbip/src/usbip_unbind.c | 4 ++--
>>>> 5 files changed, 13 insertions(+), 12 deletions(-)
>>>>
>>>> diff --git a/tools/usb/usbip/libsrc/usbip_host_common.c b/tools/usb/usbip/libsrc/usbip_host_common.c
>>>> index 9d41522..6a98d6c 100644
>>>> --- a/tools/usb/usbip/libsrc/usbip_host_common.c
>>>> +++ b/tools/usb/usbip/libsrc/usbip_host_common.c
>>>> @@ -256,17 +256,15 @@ int usbip_export_device(struct usbip_exported_device *edev, int sockfd)
>>>> }
>>>>
>>>> struct usbip_exported_device *usbip_generic_get_device(
>>>> - struct usbip_host_driver *hdriver, int num)
>>>> + struct usbip_host_driver *hdriver, char *busid)
>>>> {
>>>> struct list_head *i;
>>>> struct usbip_exported_device *edev;
>>>> - int cnt = 0;
>>>>
>>>> list_for_each(i, &hdriver->edev_list) {
>>>> edev = list_entry(i, struct usbip_exported_device, node);
>>>> - if (num == cnt)
>>>> + if (!strncmp(busid, edev->udev.busid, SYSFS_BUS_ID_SIZE))
>>>
>>> Use strlen(edev->udev.busid) instead of SYSFS_BUS_ID_SIZE
>>>
>>
>> For me using here strlen() is pointless. strncpy() is here to protect
>> against unterminated string in edev->udev.busid. In my humble opinion,
>> instead of using strncpy() all the time I would rather just ensure that
>> this string is \0 terminated in read_usb_device().
>>
>> Best regards,
>>
>
> Yes my real concern here is non-null terminated strings and also that
> edev->udev.busid can be trusted over busid. Suing edev->udev.busid
> length would only compare stelne bytes as opposed to the max. 32.
>
What do you mean as can be trusted?
busid comes from cmd line param so, assuming that libc is correct we
get this string always as a \0 terminated. In contrast udev.busid is
being strncpy() what means that it's possible that there is no \0;)
Code from usbip_common.c:
int read_usb_device(struct udev_device *sdev, struct usbip_usb_device *udev)
{
/* ... */
path = udev_device_get_syspath(sdev);
name = udev_device_get_sysname(sdev);
strncpy(udev->path, path, SYSFS_PATH_MAX);
strncpy(udev->busid, name, SYSFS_BUS_ID_SIZE);
/* ... */
}
Cheers,
--
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
