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.
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().
> > + 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.
> > +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?
Thank you for reviewing,
n.iwata
//
> -----Original Message-----
> From: Krzysztof Opasiak [mailto:k.opasiak@xxxxxxxxxxx]
> Sent: Saturday, December 03, 2016 12:46 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 07/10] usbip: exporting devices: new
> application-side daemon
>
>
>
> On 11/22/2016 07:48 AM, Nobuo Iwata wrote:
> > This patch is the new application(vhci)-side daemon specific code.
> >
> > The daemons are consisting three files.
> > usbip.c : common code.
> > usbip_dev.c: device(stub)-side specific code.
> > usbip_app.c: application(vhci)-side specific code - this patch.
> >
> > Here, device-side daemon is EXISTING-1 and application-side daemon is
> > NEW-1 in figure below.
> >
> > EXISTING) - invites devices from application(vhci)-side
> > +------+
> +------------------+
> > device--+ STUB | | application/VHCI |
> > +------+
> +------------------+
> > 1) usbipd ... start daemon
> > = = =
> > 2) usbip list --local
> > 3) usbip bind
> > <--- list bound devices --- 4) usbip list --remote
> > <--- import a device ------ 5) usbip attach
> > = = =
> > X disconnected 6) usbip detach
> > 7) usbip unbind
> >
> > NEW) - dedicates devices from device(stb)-side
> > +------+
> +------------------+
> > device--+ STUB | | application/VHCI |
> > +------+
> +------------------+
> > 1) usbipa ... start
> > daemon = = =
> > 2) usbip list --local
> > 3) usbip connect --- export a device ------>
> > = = =
> > 4) usbip disconnect --- un-export a device --->
> >
> > Signed-off-by: Nobuo Iwata <nobuo.iwata@xxxxxxxxxxxxxxx>
> > ---
> > tools/usb/usbip/libsrc/vhci_driver.c | 19 +++
> > tools/usb/usbip/libsrc/vhci_driver.h | 1 +
> > tools/usb/usbip/src/Makefile.am | 7 +-
> > tools/usb/usbip/src/usbipd.c | 12 +-
> > tools/usb/usbip/src/usbipd_app.c | 242
> +++++++++++++++++++++++++++
> > 5 files changed, 279 insertions(+), 2 deletions(-)
> >
> > diff --git a/tools/usb/usbip/libsrc/vhci_driver.c
> > b/tools/usb/usbip/libsrc/vhci_driver.c
> > index d2221c5..3fe92ff 100644
> > --- a/tools/usb/usbip/libsrc/vhci_driver.c
> > +++ b/tools/usb/usbip/libsrc/vhci_driver.c
> > @@ -314,6 +314,25 @@ int usbip_vhci_get_free_port(void)
> > return -1;
> > }
> >
> > +struct usbip_imported_device *usbip_vhci_find_device(char *host, char
> > +*busid) {
> > + int ret;
> > + char rhost[NI_MAXHOST] = "unknown host";
> > + char rserv[NI_MAXSERV] = "unknown port";
> > + char rbusid[SYSFS_BUS_ID_SIZE];
> > +
> > + for (int i = 0; i < vhci_driver->nports; i++) {
> > + ret = read_record(vhci_driver->idev[i].port, rhost,
> NI_MAXHOST,
> > + rserv, NI_MAXSERV, rbusid);
>
> sizeof(rhist), sizeof(rserv)
>
> 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:(
>
>
> > + 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?
> I also thing that it's better to ensure that strings are \0 terminated rather
> than using strncmp all the time.
>
> > + }
> > + return NULL;
> > +}
> > +
>
> (...)
>
> > +
> > +static int import_device(int sockfd, struct usbip_usb_device *udev) {
> > + int rc;
> > + int port;
> > +
> > + dbg("Sockfd:%d", sockfd);
> > + port = usbip_vhci_get_free_port();
> > + if (port < 0) {
> > + err("no free port");
> > + return -1;
> > + }
> > +
> > + dump_usb_device(udev);
> > + rc = usbip_vhci_attach_device(port, sockfd, udev->busnum,
> > + udev->devnum, udev->speed);
> > + if (rc < 0) {
> > + err("import device");
> > + return -1;
> > + }
> > +
> > + return port;
> > +}
> > +
> > +static int recv_request_export(int sockfd, char *host, char *port) {
> > + struct op_export_request req;
> > + struct op_export_reply reply;
> > + int rhport = 0;
> > + int error = 0;
> > + int rc;
> > +
> > + memset(&req, 0, sizeof(req));
> > + memset(&reply, 0, sizeof(reply));
>
> As before. You don't need to memset() reply.
>
> > +
> > + rc = usbip_net_recv(sockfd, &req, sizeof(req));
> > + if (rc < 0) {
> > + dbg("usbip_net_recv failed: export request");
> > + return -1;
> > + }
> > + PACK_OP_EXPORT_REQUEST(0, &req);
> > +
> > + rhport = import_device(sockfd, &req.udev);
> > + if (rhport < 0) {
> > + dbg("export request busid %s: failed", req.udev.busid);
> > + error = 1;
> > + }
> > +
> > + rc = usbip_net_send_op_common(sockfd, OP_REP_EXPORT,
> > + (!error ? ST_OK : ST_NA));
> > + if (rc < 0) {
> > + dbg("usbip_net_send_op_common failed: %#0x",
> OP_REP_EXPORT);
> > + return -1;
> > + }
> > +
> > + if (!error)
> > + reply.returncode = 0;
> > + else
> > + reply.returncode = -1;
>
> As I wrote in first patch. This may end up in compiler warning
>
> > +
> > + PACK_OP_EXPORT_REPLY(0, &rep);
> > +
> > + rc = usbip_net_send(sockfd, &reply, sizeof(reply));
> > + if (rc < 0) {
> > + dbg("usbip_net_send failed: export reply");
> > + return -1;
> > + }
> > +
> > + 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.
>
> > + if (rc < 0) {
> > + err("record connection");
> > + return -1;
> > + }
> > +
> > + dbg("export request busid %s: complete", req.udev.busid);
> > +
> > + return 0;
> > +}
> > +
> > +static int unimport_device(char *host, struct usbip_usb_device *udev)
> > +{
> > + int rc;
> > + struct usbip_imported_device *idev;
> > +
> > + idev = usbip_vhci_find_device(host, udev->busid);
> > + if (idev == NULL) {
> > + err("no imported port %s %s", host, udev->busid);
> > + return -1;
> > + }
> > +
> > + rc = usbip_vhci_detach_device(idev->port);
> > + if (rc < 0) {
> > + err("no imported port %d %s %s", idev->port, host,
> udev->busid);
> > + return -1;
> > + }
> > + return idev->port;
> > +}
> > +
> > +static int recv_request_unexport(int sockfd, char *host) {
> > + struct op_unexport_request req;
> > + struct op_unexport_reply reply;
> > + int rhport = 0;
> > + int error = 0;
> > + int rc;
> > +
> > + memset(&req, 0, sizeof(req));
> > + memset(&reply, 0, sizeof(reply));
>
> As before. You don't need to memset() reply.
>
> > +
> > + rc = usbip_net_recv(sockfd, &req, sizeof(req));
> > + if (rc < 0) {
> > + dbg("usbip_net_recv failed: unexport request");
> > + return -1;
> > + }
> > + PACK_OP_UNEXPORT_REQUEST(0, &req);
> > +
> > + rhport = unimport_device(host, &req.udev);
> > + if (rhport < 0)
> > + error = 1;
> > +
> > + rc = usbip_net_send_op_common(sockfd, OP_REP_UNEXPORT,
> > + (!error ? ST_OK : ST_NA));
> > + if (rc < 0) {
> > + dbg("usbip_net_send_op_common failed: %#0x",
> OP_REP_UNEXPORT);
> > + return -1;
> > + }
> > +
> > + if (!error) {
> > + reply.returncode = 0;
> > + } else {
> > + reply.returncode = -1;
>
> As I wrote in first patch. This may end up in compiler warning
>
> > + dbg("unexport request busid %s: failed", req.udev.busid);
> > + return -1;
> > + }
> > + PACK_OP_UNEXPORT_REPLY(0, &rep);
> > +
> > + rc = usbip_net_send(sockfd, &reply, sizeof(reply));
> > + if (rc < 0) {
> > + dbg("usbip_net_send failed: unexport reply");
> > + return -1;
> > + }
> > +
> > + 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.
>
> > +
> > + dbg("unexport request busid %s: complete", req.udev.busid);
> > +
> > + return 0;
> > +}
> > +
> > +int usbip_recv_pdu(int connfd, char *host, char *port) {
> > + uint16_t code = OP_UNSPEC;
> > + int ret;
> > +
> > + ret = usbip_net_recv_op_common(connfd, &code);
> > + if (ret < 0) {
> > + dbg("could not receive opcode: %#0x", code);
> > + return -1;
> > + }
> > +
> > + ret = usbip_vhci_refresh_device_list();
> > + if (ret < 0) {
> > + dbg("could not refresh device list: %d", ret);
> > + return -1;
> > + }
> > +
> > + info("received request: %#0x(%d)", code, connfd);
> > + switch (code) {
> > + case OP_REQ_EXPORT:
> > + ret = recv_request_export(connfd, host, port);
> > + break;
> > + case OP_REQ_UNEXPORT:
> > + ret = recv_request_unexport(connfd, host);
> > + break;
> > + default:
> > + err("received an unknown opcode: %#0x", code);
> > + ret = -1;
> > + }
> > +
> > + if (ret == 0)
> > + info("request %#0x(%s:%s): complete", code, host, port);
> > + else
> > + info("request %#0x(%s:%s): failed", code, host, port);
> > +
> > + return ret;
> > +}
> >
>
> 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?
>
> 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
