> > + memset(&request, 0, sizeof(request));
> > + memset(&reply, 0, sizeof(reply));
>
> As in previous patch, you don't need to 0 the reply.
I will remove the memset.
> sizeof(request.udev)?
I will modify.
> > + /* check the reply */
> > + if (reply.returncode) {
> > + err("recv error return %d", reply.returncode);
> > + return -1;
> > + }
>
> The same case with error code as in previous patch.
It will be removed.
> Once again the same pattern as in previous patch 3 times in a row copy-pasted
> error path. Please use goto here and place error path below return.
I will fix it.
> close(sockfd) in case of error, otherwise close(sockfd)...
>
> Probably you can place close(sockfd) above if to avoid this weird
> copy-paste.
I will fix it.
> > + ret = disconnect_device(host, busid, unbind);
> > + goto out;
>
> This looks a little bit weird... why you use goto out instead of just return
> ret?
err_out has usbip_disconnect_usage().
Each usbip_<something>.c has same pattern.
Thank you,
n.iwata
//
> -----Original Message-----
> From: Krzysztof Opasiak [mailto:k.opasiak@xxxxxxxxxxx]
> Sent: Friday, December 02, 2016 5:37 AM
> To: fx IWATA NOBUO; valentina.manea.m@xxxxxxxxx; shuah.kh@xxxxxxxxxxx;
> gregkh@xxxxxxxxxxxxxxxxxxx; linux-usb@xxxxxxxxxxxxxxx
> Cc: fx MICHIMURA TADAO
> Subject: Re: [PATCH v13 04/10] usbip: exporting devices: new disconnect
> operation
>
>
>
> On 11/22/2016 07:48 AM, Nobuo Iwata wrote:
> (...)
> > +
> > +static int send_unexport_device(int sockfd, struct usbip_usb_device
> > +*udev) {
> > + int rc;
> > + struct op_unexport_request request;
> > + struct op_unexport_reply reply;
> > + uint16_t code = OP_REP_UNEXPORT;
> > +
> > + memset(&request, 0, sizeof(request));
> > + memset(&reply, 0, sizeof(reply));
>
> As in previous patch, you don't need to 0 the reply.
>
> > +
> > + /* send a request */
> > + rc = usbip_net_send_op_common(sockfd, OP_REQ_UNEXPORT, 0);
> > + if (rc < 0) {
> > + err("send op_common");
> > + return -1;
> > + }
> > +
> > + memcpy(&request.udev, udev, sizeof(struct usbip_usb_device));
>
> sizeof(request.udev)?
>
> > +
> > + PACK_OP_UNEXPORT_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_unexport_reply");
> > + return -1;
> > + }
> > +
> > + PACK_OP_EXPORT_REPLY(0, &reply);
> > +
> > + /* check the reply */
> > + if (reply.returncode) {
> > + err("recv error return %d", reply.returncode);
> > + return -1;
> > + }
>
> The same case with error code as in previous patch.
>
> > +
> > + return 0;
> > +}
> > +
> > +static int unexport_device(char *busid, int sockfd) {
> > + int rc;
> > + struct usbip_exported_device *edev;
> > +
> > + rc = usbip_driver_open(driver);
> > + if (rc < 0) {
> > + 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_unexport_device(sockfd, &edev->udev);
> > + if (rc < 0) {
> > + err("send unexport");
> > + usbip_driver_close(driver);
> > + return -1;
> > + }
>
> Once again the same pattern as in previous patch 3 times in a row copy-pasted
> error path. Please use goto here and place error path below return.
>
> > +
> > + usbip_driver_close(driver);
> > +
> > + return 0;
> > +}
> > +
> > +static int disconnect_device(char *host, char *busid, int unbind) {
> > + int sockfd;
> > + int rc;
> > +
> > + sockfd = usbip_net_tcp_connect(host, usbip_port_string);
> > + if (sockfd < 0) {
> > + err("tcp connect");
> > + return -1;
> > + }
> > +
> > + rc = unexport_device(busid, sockfd);
> > + if (rc < 0) {
> > + err("unexport");
> > + close(sockfd);
> > + return -1;
> > + }
>
> close(sockfd) in case of error, otherwise close(sockfd)...
>
> Probably you can place close(sockfd) above if to avoid this weird
> copy-paste.
>
> > +
> > + close(sockfd);
> > +
> > + if (unbind) {
> > + rc = usbip_unbind_device(busid);
> > + if (rc) {
> > + err("unbind");
> > + return -1;
> > + }
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +int usbip_disconnect(int argc, char *argv[]) {
> > + static const struct option opts[] = {
> > + { "remote", required_argument, NULL, 'r' },
> > + { "busid", required_argument, NULL, 'b' },
> > + { "device", no_argument, NULL, 'd' },
> > + { NULL, 0, NULL, 0 }
> > + };
> > + char *host = NULL;
> > + char *busid = NULL;
> > + int opt;
> > + int unbind = 1;
> > + int ret = -1;
> > +
> > + for (;;) {
> > + opt = getopt_long(argc, argv, "r:b:d", opts, NULL);
> > +
> > + if (opt == -1)
> > + break;
> > +
> > + switch (opt) {
> > + case 'r':
> > + host = optarg;
> > + break;
> > + case 'b':
> > + busid = optarg;
> > + break;
> > + case 'd':
> > + driver = &device_driver;
> > + unbind = 0;
> > + break;
> > + default:
> > + goto err_out;
> > + }
> > + }
> > +
> > + if (!host || !busid)
> > + goto err_out;
> > +
> > + ret = disconnect_device(host, busid, unbind);
> > + goto out;
>
> This looks a little bit weird... why you use goto out instead of just return
> ret?
>
> > +
> > +err_out:
> > + usbip_disconnect_usage();
> > +out:
> > + return ret;
> > +}
> >
>
> 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
