Hello,
> In my humble opinion connect is not a best name for this operation.
Yes. I think connect is not perfect.
> it not only starts the connection to a remote server but also> exports
> a device. I think that a name like export or attach_to_remote would be
> more suitable. (...)
I think one word name is better.
Considering that attach is corresponding to import request, I think
export is not good. Furthermore, export is already used in many places
of the code, ex. usbip_host_common.c: usbip_export_device(). So I don't
want to use export.
Operation | PDU | Description
----------+---------+-----------------
attach |import |invite a device
detach |NA |quit invited
( ) |export |dedicate a device
( ) |un-export|quit dedicated
I think connect/disconnect is not bad.
I will change it if there's user friendly, well-known and suit with
existing name.
> > + memset(&request, 0, sizeof(request));
> > + memset(&reply, 0, sizeof(reply));
>
> Probably you don't need to 0 the reply as it is buffer for receiving
> and it's not being send anywhere.
I will remove it.
I will remove from send_unexport_device() in 4/10 too.
usbip_attach.c:query_import_device() has same memset.
I will fix it outside of this patch set.
> > + memcpy(&request.udev, udev, sizeof(struct usbip_usb_device));
>
> how about sizeof(request.udev)?
I will change it.
I will grep sizeof and change to fit to nearby them.
> > + /* check the reply */
> > + if (reply.returncode) {
> > + err("recv error return %d", reply.returncode);
> > + return -1;
> > + }
>
> This check should probably look different or at least should be
> documented. Please refer to my comments to patch #1.
It will be removed.
> Take a look at all above function calls and your if.
>
> You have usbip_driver_close() called 4 times in 4 error path. This is
> just asking to replace this with goto and place error path below
> return just like you did one function below.
I will fix it.
usbip_attach.c has same logic so I will add refactoring patch to
usbip_attach.c. regarding memset() and multiple driver_close.
Thank you for your help,
n.iwata
//
> -----Original Message-----
> From: Krzysztof Opasiak [mailto:[email protected]]
> Sent: Friday, December 02, 2016 5:28 AM
> To: fx IWATA NOBUO; [email protected]; [email protected];
> [email protected]; [email protected]
> Cc: fx MICHIMURA TADAO
> Subject: Re: [PATCH v13 03/10] usbip: exporting devices: new connect
> operation
>
> Hi,
>
> On 11/22/2016 07:48 AM, Nobuo Iwata wrote:
> > Implementation of new connect operation. This is linked as a part of
> > usbip commnad. With this patch, usbip command has following operations.
> >
> > bind
> > unbind
> > list (local/remote)
> > attach
> > detach
> > port
> > connect ... this patch
>
>
> In my humble opinion connect is not a best name for this operation.
> it not only starts the connection to a remote server but also exports a
> device. I think that a name like export or attach_to_remote would be more
> suitable.
> (...)
>
> > +static const char usbip_connect_usage_string[] =
> > + "usbip connect <args>\n"
> > + " -r, --remote=<host> Address of a remote computer\n"
> > + " -b, --busid=<busid> Bus ID of a device to be connected\n"
> > + " -d, --device Run with an alternate driver, e.g.
> vUDC\n";
> > +
> > +void usbip_connect_usage(void)
> > +{
> > + printf("usage: %s", usbip_connect_usage_string); }
> > +
> > +static int send_export_device(int sockfd, struct usbip_usb_device
> > +*udev) {
> > + int rc;
> > + struct op_export_request request;
> > + struct op_export_reply reply;
> > + uint16_t code = OP_REP_EXPORT;
> > +
> > + memset(&request, 0, sizeof(request));
> > + memset(&reply, 0, sizeof(reply));
>
> Probably you don't need to 0 the reply as it is buffer for receiving and
> it's not being send anywhere.
>
> > +
> > + /* send a request */
> > + rc = usbip_net_send_op_common(sockfd, OP_REQ_EXPORT, 0);
> > + if (rc < 0) {
> > + err("send op_common");
> > + return -1;
> > + }
> > +
> > + memcpy(&request.udev, udev, sizeof(struct usbip_usb_device));
>
> how about sizeof(request.udev)?
>
> > +
> > + PACK_OP_EXPORT_REQUEST(0, &request);
> > +
> > + rc = usbip_net_send(sockfd, (void *) &request, sizeof(request));
> > + if (rc < 0) {
> > + err("send op_export_request");
> > + return -1;
> > + }
> > +
> > + /* receive a reply */
> > + rc = usbip_net_recv_op_common(sockfd, &code);
> > + if (rc < 0) {
> > + err("recv op_common");
> > + return -1;
> > + }
> > +
> > + rc = usbip_net_recv(sockfd, (void *) &reply, sizeof(reply));
> > + if (rc < 0) {
> > + err("recv op_export_reply");
> > + return -1;
> > + }
> > +
> > + PACK_OP_EXPORT_REPLY(0, &reply);
> > +
> > + /* check the reply */
> > + if (reply.returncode) {
> > + err("recv error return %d", reply.returncode);
> > + return -1;
> > + }
>
> This check should probably look different or at least should be documented.
> Please refer to my comments to patch #1.
>
> > +
> > + return 0;
> > +}
> > +
> > +static int export_device(char *busid, int sockfd) {
> > + int rc;
> > + struct usbip_exported_device *edev;
> > +
> > + rc = usbip_driver_open(driver);
> > + if (rc) {
> > + err("open driver");
> > + return -1;
> > + }
> > +
> > + rc = usbip_refresh_device_list(driver);
> > + if (rc < 0) {
> > + err("could not refresh device list");
> > + usbip_driver_close(driver);
> > + return -1;
> > + }
> > +
> > + edev = usbip_get_device(driver, busid);
> > + if (edev == NULL) {
> > + err("find device");
> > + usbip_driver_close(driver);
> > + return -1;
> > + }
> > +
> > + rc = send_export_device(sockfd, &edev->udev);
> > + if (rc < 0) {
> > + err("send export");
> > + usbip_driver_close(driver);
> > + return -1;
> > + }
> > +
> > + rc = usbip_export_device(edev, sockfd);
> > + if (rc < 0) {
> > + err("export device");
> > + usbip_driver_close(driver);
> > + return -1;
> > + }
>
> Take a look at all above function calls and your if.
>
> You have usbip_driver_close() called 4 times in 4 error path. This is just
> asking to replace this with goto and place error path below return just
> like you did one function below.
>
> Not to mention that you should propagate the error code to caller.
>
> > +
> > + usbip_driver_close(driver);
> > +
> > + return 0;
> > +}
>
> 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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
