Sorry for spam but previous mail didn't reach linux-usb due to some
problems with my company mail server.
On 12/06/2016 10:48 AM, Krzysztof Opasiak wrote:
> Hi,
>
> On 12/06/2016 09:44 AM, fx IWATA NOBUO wrote:
>> Hello,
>>
>>> sizeof(rhist), sizeof(rserv)
>>
>> I will fix them.
>>
>>> BTW.
>>> The read_record() function is really weird and probably it should be
>>> also refactored. We pass 3 arrays but pass size only for 2 of them and
>>> size of third argument is hard coded inside this function:(
>>
>> Comment "the 3rd part without it being truncated to an acceptable
>> length" at read_record of original code.
>>
>> But I will add busid_len and modify the comment.
>> Also I will check port operation's output will not be changed.
>>
>>>> + if (!ret &&
>>>> + !strncmp(host, rhost, NI_MAXHOST) &&
>>>> + !strncmp(busid, rbusid, SYSFS_BUS_ID_SIZE)) {
>>>> + return vhci_driver->idev + i;
>>>> + }
>>>
>>> Is there any reason why you don't check port here?
>>
>> Remote port number will be different in each connection.
>> This function checks if remote device of remote host is already imported.
>> So it's not checked.
>
> You are absolutely right here. Thank you for your explanation.
>
>>
>> I will refactor read_record() that it can omit if the pointer is NULL.
>>
>>> I also thing that it's better to ensure that strings are \0 terminated
>>> rather than using strncmp all the time.
>>
>> getnameinfo(3), getaddrinfo(3) always set NULL terminator and NI_MAX_
>> includes the NULL.
>>
>> So I will make read_record() always set NULL terminator.
>> Also add comment at read_record().
>
> Please do this in a separate series with fixes and improvements.
>
>>
>>>> + memset(&req, 0, sizeof(req));
>>>> + memset(&reply, 0, sizeof(reply));
>>>
>>> As before. You don't need to memset() reply.
>>
>> I will remove it.
>>
>>>> + if (!error)
>>>> + reply.returncode = 0;
>>>> + else
>>>> + reply.returncode = -1;
>>>
>>> As I wrote in first patch. This may end up in compiler warning
>>
>> The field return code will be removed.
>>
>>>> + rc = usbip_vhci_create_record(host, port, req.udev.busid,
>>> rhport);
>>>
>>> This call should be moved to import_device() as we may endup with
>>> inconsistent_state() if there is a comminication error.
>>
>> Yes!
>> It must be match with the result of vhci_attach_device().
>>
>>>> + usbip_vhci_delete_record(rhport);
>>>
>>> I think that this function should be called from unimport_device()
>>>
>>> Now this function can be ommited if there is some communication error and
>>> we may endup in inconsistent state.
>>
>> I will fix to match the status of vhci_detach_device().
>>
>> I found original usbip_attach() and usbip_detach() have same problem.
>> I will fix them patch 6/10: refactoring of them.
>
> Please do this in a separate series so it may be applied as is without
> discussing device exporting feature.
>
>>
>>>> +int usbip_recv_pdu(int connfd, char *host, char *port) {
>>>> --snip--
>>>> +}
>>> This one is a copy-paste from usbip_recv_pdu() in usbipd_dev.c with
>>> only values changed in switch. How about sharing this code and add
>>> only callback which will be used based on received op_code?
>>
>> usbip_refresh_device_list(), usbip_vhci_refresh_device_list() must be
>> wrapped as same as open, close.
>>
>> Is usbip_meta_refresh_device_list() acceptable?
>
> Sounds good. Let's try.
>
> 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
