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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
