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