Hello, > > -static int do_accept(int listenfd) > > +static int do_accept(int listenfd, char *host, char *port) > In my opinion you should pass also the size of those arrays to this > function instead of hardcoding them like this. OK. I will add them. > > - if (usbip_driver_open(driver)) > > + if (usbip_open_driver()) > > What's the purpose of this function? > > - usbip_driver_close(driver); > > + usbip_close_driver(); > > As above? Why not make this a global variable and leave those calls as > they were? usbip_driver_open/close - host side abstraction including stub and vudc. usbip_open/close_driver - abstraction both host(stub/vudc) and vhci. usbip_driver_<something> has widely used as function names and file names for host side abstraction. If they were usbip_host_<something>, I can use usbip_driver_open/close for both host(stub/vudc) and vhci. > I totally don't understand why those functions exist? > Their names are very confusing and they don't implement any additional > functionality. The abstraction is shared with patch 7/10. > > - driver = &device_driver; > > + usbip_update_driver(); > > usbip_update_driver() is t totally unrelated to what this assignment > really does. > > It changes the backend from stub driver to vudc not updating the driver. It used to make symmetric device side and application side. In device side, it switches to device driver. In application side, no operation. > usbip_dev.c is not a best name for this file. It causes confusion (at least > to me) as I don't know to what the dev part is related, usb device, device > (stub side) or what? Device side. > > + memset(&req, 0, sizeof(req)); > > + memset(&reply, 0, sizeof(reply)); > > You don't need to 0 the reply. This is moved from usbipd.c. I will remove. > > + PACK_OP_DEVLIST_REPLY(1, &reply); > Could you please make the defines for 1 and 0 in this macro? > As far as I understand the code it means pack and unpack but it would > be much more readable if we have a define for this. Here's the list in the original code. usbip_attach.c: PACK_OP_IMPORT_REQUEST(0, &request); usbip_attach.c: PACK_OP_IMPORT_REPLY(0, &reply); usbip_list.c: PACK_OP_DEVLIST_REPLY(0, &reply); usbip_network.c: PACK_OP_COMMON(1, &op_common); usbip_network.c: PACK_OP_COMMON(0, &op_common); usbipd.c: PACK_OP_IMPORT_REQUEST(0, &req); usbipd.c: PACK_OP_DEVLIST_REPLY(1, &reply); Could I fix them in a refactoring patch including memset(0) and etc in files tot touched in this set or outside of this set? PACK_OP_PACK 1 PACK_OP_UNPACK 0 > are we waiting here for message of size 0 or I missed something? No. It's just moved from usbipd.c. I will remove it. Thank you, n.iwata // > -----Original Message----- > From: Krzysztof Opasiak [mailto:k.opasiak@xxxxxxxxxxx] > Sent: Friday, December 02, 2016 6:19 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 05/10] usbip: exporting devices: modifications to > daemon > > > Hi, > > On 11/22/2016 07:48 AM, Nobuo Iwata wrote: > (...) > > > > > -static int do_accept(int listenfd) > > +static int do_accept(int listenfd, char *host, char *port) > > { > > int connfd; > > struct sockaddr_storage ss; > > socklen_t len = sizeof(ss); > > - char host[NI_MAXHOST], port[NI_MAXSERV]; > > int rc; > > > > memset(&ss, 0, sizeof(ss)); > > @@ -319,8 +124,8 @@ static int do_accept(int listenfd) > > return -1; > > } > > > > - rc = getnameinfo((struct sockaddr *)&ss, len, host, sizeof(host), > > - port, sizeof(port), NI_NUMERICHOST | > NI_NUMERICSERV); > > + rc = getnameinfo((struct sockaddr *)&ss, len, host, NI_MAXHOST, > > + port, NI_MAXSERV, NI_NUMERICHOST | > NI_NUMERICSERV); > > You have removed the host and port variables from here and placed them in > caller of this function, but you left here sizes of that variables. > > It's not very good as at some point we may change the size of host and port > arrays and can easily forget to change those sizes here. > > In my opinion you should pass also the size of those arrays to this function > instead of hardcoding them like this. > > > if (rc) > > err("getnameinfo: %s", gai_strerror(rc)); > > > > @@ -334,6 +139,9 @@ static int do_accept(int listenfd) #endif > > info("connection from %s:%s", host, port); > > > > + /* should set TCP_NODELAY for usbip */ > > + usbip_net_set_nodelay(connfd); > > + > > return connfd; > > } > > > > @@ -341,14 +149,15 @@ int process_request(int listenfd) { > > pid_t childpid; > > int connfd; > > + char host[NI_MAXHOST], port[NI_MAXSERV]; > > > > - connfd = do_accept(listenfd); > > + connfd = do_accept(listenfd, host, port); > > if (connfd < 0) > > return -1; > > childpid = fork(); > > if (childpid == 0) { > > close(listenfd); > > - recv_pdu(connfd); > > + usbip_recv_pdu(connfd, host, port); > > exit(0); > > } > > close(connfd); > > @@ -496,13 +305,13 @@ static int do_standalone_mode(int daemonize, int > ipv4, int ipv6) > > struct timespec timeout; > > sigset_t sigmask; > > > > - if (usbip_driver_open(driver)) > > + if (usbip_open_driver()) > > What's the purpose of this function? > > > return -1; > > > > if (daemonize) { > > if (daemon(0, 0) < 0) { > > err("daemonizing failed: %s", strerror(errno)); > > - usbip_driver_close(driver); > > + usbip_close_driver(); > > return -1; > > } > > umask(0); > > @@ -511,7 +320,7 @@ static int do_standalone_mode(int daemonize, int ipv4, > int ipv6) > > set_signal(); > > write_pid_file(); > > > > - info("starting " PROGNAME " (%s)", usbip_version_string); > > + info("starting %s (%s)", usbip_progname, usbip_version_string); > > > > /* > > * To suppress warnings on systems with bindv6only disabled @@ > > -527,7 +336,7 @@ static int do_standalone_mode(int daemonize, int > > ipv4, int ipv6) > > > > ai_head = do_getaddrinfo(NULL, family); > > if (!ai_head) { > > - usbip_driver_close(driver); > > + usbip_close_driver(); > > As above? Why not make this a global variable and leave those calls as they > were? > > > return -1; > > } > > nsockfd = listen_all_addrinfo(ai_head, sockfdlist, @@ -535,7 > +344,7 > > @@ static int do_standalone_mode(int daemonize, int ipv4, int ipv6) > > freeaddrinfo(ai_head); > > if (nsockfd <= 0) { > > err("failed to open a listening socket"); > > - usbip_driver_close(driver); > > + usbip_close_driver(); > > return -1; > > } > > > > @@ -574,9 +383,9 @@ static int do_standalone_mode(int daemonize, int ipv4, > int ipv6) > > } > > } > > > > - info("shutting down " PROGNAME); > > + info("shutting down %s", usbip_progname); > > free(fds); > > - usbip_driver_close(driver); > > + usbip_close_driver(); > > > > return 0; > > } > > @@ -587,7 +396,6 @@ int main(int argc, char *argv[]) > > { "ipv4", no_argument, NULL, '4' }, > > { "ipv6", no_argument, NULL, '6' }, > > { "daemon", no_argument, NULL, 'D' }, > > - { "daemon", no_argument, NULL, 'D' }, > > { "debug", no_argument, NULL, 'd' }, > > { "device", no_argument, NULL, 'e' }, > > { "pid", optional_argument, NULL, 'P' }, > > @@ -616,7 +424,7 @@ int main(int argc, char *argv[]) > > err("not running as root?"); > > > > cmd = cmd_standalone_mode; > > - driver = &host_driver; > > + usbip_init_driver(); > > for (;;) { > > opt = getopt_long(argc, argv, "46DdeP::t:hv", longopts, > NULL); > > > > @@ -640,7 +448,7 @@ int main(int argc, char *argv[]) > > cmd = cmd_help; > > break; > > case 'P': > > - pid_file = optarg ? optarg : DEFAULT_PID_FILE; > > + pid_file = optarg ? optarg : > usbip_default_pid_file; > > break; > > case 't': > > usbip_setup_port_number(optarg); > > @@ -649,7 +457,7 @@ int main(int argc, char *argv[]) > > cmd = cmd_version; > > break; > > case 'e': > > - driver = &device_driver; > > + usbip_update_driver(); > > usbip_update_driver() is t totally unrelated to what this assignment really > does. > > It changes the backend from stub driver to vudc not updating the driver. > > > break; > > case '?': > > usbipd_help(); > > @@ -667,7 +475,7 @@ int main(int argc, char *argv[]) > > remove_pid_file(); > > break; > > case cmd_version: > > - printf(PROGNAME " (%s)\n", usbip_version_string); > > + printf("%s (%s)\n", usbip_progname, > usbip_version_string); > > rc = 0; > > break; > > case cmd_help: > > diff --git a/tools/usb/usbip/src/usbipd.h > > b/tools/usb/usbip/src/usbipd.h new file mode 100644 index > > 0000000..df514f7 > > --- /dev/null > > +++ b/tools/usb/usbip/src/usbipd.h > > @@ -0,0 +1,39 @@ > > +/* > > + * Copyright (C) 2015 Nobuo Iwata > > + * 2011 matt mooney <mfm@xxxxxxxxxxxxx> > > + * 2005-2007 Takahiro Hirofuchi > > + * > > + * This program is free software: you can redistribute it and/or > > +modify > > + * it under the terms of the GNU General Public License as published > > +by > > + * the Free Software Foundation, either version 2 of the License, or > > + * (at your option) any later version. > > + * > > + * This program is distributed in the hope that it will be useful, > > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > > + * GNU General Public License for more details. > > + * > > + * You should have received a copy of the GNU General Public License > > + * along with this program. If not, see <http://www.gnu.org/licenses/>. > > + */ > > + > > +#ifndef __USBIPD_H > > +#define __USBIPD_H > > + > > +#include "usbip_common.h" > > + > > +#ifdef HAVE_CONFIG_H > > +#include "../config.h" > > +#endif > > + > > +extern char *usbip_progname; > > +extern char *usbip_default_pid_file; > > + > > +int usbip_recv_pdu(int connfd, char *host, char *port); > > + > > +void usbip_init_driver(void); > > +void usbip_update_driver(void); > > +int usbip_open_driver(void); > > +void usbip_close_driver(void); > > + > > +#endif /* __USBIPD_H */ > > diff --git a/tools/usb/usbip/src/usbipd_dev.c > > b/tools/usb/usbip/src/usbipd_dev.c > > usbip_dev.c is not a best name for this file. It causes confusion (at least > to me) as I don't know to what the dev part is related, usb device, device > (stub side) or what? > > > new file mode 100644 > > index 0000000..33f6c22 > > --- /dev/null > > +++ b/tools/usb/usbip/src/usbipd_dev.c > > @@ -0,0 +1,252 @@ > > +/* > > + * Copyright (C) 2015 Nobuo Iwata > > + * 2011 matt mooney <mfm@xxxxxxxxxxxxx> > > + * 2005-2007 Takahiro Hirofuchi > > + * > > + * This program is free software: you can redistribute it and/or > > +modify > > + * it under the terms of the GNU General Public License as published > > +by > > + * the Free Software Foundation, either version 2 of the License, or > > + * (at your option) any later version. > > + * > > + * This program is distributed in the hope that it will be useful, > > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > > + * GNU General Public License for more details. > > + * > > + * You should have received a copy of the GNU General Public License > > + * along with this program. If not, see <http://www.gnu.org/licenses/>. > > + */ > > + > > +#ifdef HAVE_CONFIG_H > > +#include "../config.h" > > +#endif > > + > > +#define _GNU_SOURCE > > +#include <errno.h> > > +#include <unistd.h> > > +#include <netdb.h> > > +#include <string.h> > > +#include <stdlib.h> > > +#include <sys/types.h> > > +#include <sys/stat.h> > > +#include <arpa/inet.h> > > +#include <sys/socket.h> > > +#include <netinet/in.h> > > + > > +#include "usbip_host_driver.h" > > +#include "usbip_host_common.h" > > +#include "usbip_device_driver.h" > > +#include "usbip_common.h" > > +#include "usbip_network.h" > > +#include "list.h" > > + > > +char *usbip_progname = "usbipd"; > > +char *usbip_default_pid_file = "/var/run/usbipd"; > > + > > +static struct usbip_host_driver *driver; > > + > > +void usbip_init_driver(void) > > +{ > > + driver = &host_driver; > > +} > > + > > +void usbip_update_driver(void) > > +{ > > + driver = &device_driver; > > +} > > + > > +int usbip_open_driver(void) > > +{ > > + return usbip_driver_open(driver); > > +} > > + > > +void usbip_close_driver(void) > > +{ > > + usbip_driver_close(driver); > > +} > > + > > I totally don't understand why those functions exist? > Their names are very confusing and they don't implement any additional > functionality. > > > +static int recv_request_import(int sockfd) { > > + struct op_import_request req; > > + struct op_common reply; > > + struct usbip_exported_device *edev; > > + struct usbip_usb_device pdu_udev; > > + struct list_head *i; > > + int found = 0; > > + int error = 0; > > + int rc; > > + > > + memset(&req, 0, sizeof(req)); > > + memset(&reply, 0, sizeof(reply)); > > You don't need to 0 the reply. > > > + > > + rc = usbip_net_recv(sockfd, &req, sizeof(req)); > > + if (rc < 0) { > > + dbg("usbip_net_recv failed: import request"); > > + return -1; > > + } > > + PACK_OP_IMPORT_REQUEST(0, &req); > > + > > + list_for_each(i, &driver->edev_list) { > > + edev = list_entry(i, struct usbip_exported_device, node); > > + if (!strncmp(req.busid, edev->udev.busid, > SYSFS_BUS_ID_SIZE)) { > > + info("found requested device: %s", req.busid); > > + found = 1; > > + break; > > + } > > + } > > + > > + if (found) { > > + /* export device needs a TCP/IP socket descriptor */ > > + rc = usbip_export_device(edev, sockfd); > > + if (rc < 0) > > + error = 1; > > + } else { > > + info("requested device not found: %s", req.busid); > > + error = 1; > > + } > > + > > + rc = usbip_net_send_op_common(sockfd, OP_REP_IMPORT, > > + (!error ? ST_OK : ST_NA)); > > + if (rc < 0) { > > + dbg("usbip_net_send_op_common failed: %#0x", > OP_REP_IMPORT); > > + return -1; > > + } > > + > > + if (error) { > > + dbg("import request busid %s: failed", req.busid); > > + return -1; > > + } > > + > > + memcpy(&pdu_udev, &edev->udev, sizeof(pdu_udev)); > > + usbip_net_pack_usb_device(1, &pdu_udev); > > + > > + rc = usbip_net_send(sockfd, &pdu_udev, sizeof(pdu_udev)); > > + if (rc < 0) { > > + dbg("usbip_net_send failed: devinfo"); > > + return -1; > > + } > > + > > + dbg("import request busid %s: complete", req.busid); > > + > > + return 0; > > +} > > + > > +static int send_reply_devlist(int connfd) { > > + struct usbip_exported_device *edev; > > + struct usbip_usb_device pdu_udev; > > + struct usbip_usb_interface pdu_uinf; > > + struct op_devlist_reply reply; > > + struct list_head *j; > > + int rc, i; > > + > > + reply.ndev = 0; > > + /* number of exported devices */ > > + list_for_each(j, &driver->edev_list) { > > + reply.ndev += 1; > > + } > > why not just driver.ndevs? > > > + info("exportable devices: %d", reply.ndev); > > + > > + rc = usbip_net_send_op_common(connfd, OP_REP_DEVLIST, ST_OK); > > + if (rc < 0) { > > + dbg("usbip_net_send_op_common failed: %#0x", > OP_REP_DEVLIST); > > + return -1; > > + } > > + PACK_OP_DEVLIST_REPLY(1, &reply); > > Could you please make the defines for 1 and 0 in this macro? > As far as I understand the code it means pack and unpack but it would be > much more readable if we have a define for this. > > > + > > + rc = usbip_net_send(connfd, &reply, sizeof(reply)); > > + if (rc < 0) { > > + dbg("usbip_net_send failed: %#0x", OP_REP_DEVLIST); > > + return -1; > > + } > > + > > + list_for_each(j, &driver->edev_list) { > > + edev = list_entry(j, struct usbip_exported_device, node); > > + dump_usb_device(&edev->udev); > > + memcpy(&pdu_udev, &edev->udev, sizeof(pdu_udev)); > > + usbip_net_pack_usb_device(1, &pdu_udev); > > + > > + rc = usbip_net_send(connfd, &pdu_udev, > sizeof(pdu_udev)); > > + if (rc < 0) { > > + dbg("usbip_net_send failed: pdu_udev"); > > + return -1; > > + } > > + > > + for (i = 0; i < edev->udev.bNumInterfaces; i++) { > > + dump_usb_interface(&edev->uinf[i]); > > + memcpy(&pdu_uinf, &edev->uinf[i], > sizeof(pdu_uinf)); > > + usbip_net_pack_usb_interface(1, &pdu_uinf); > > + > > + rc = usbip_net_send(connfd, &pdu_uinf, > > + sizeof(pdu_uinf)); > > + if (rc < 0) { > > + err("usbip_net_send failed: pdu_uinf"); > > + return -1; > > + } > > + } > > + } > > + > > + return 0; > > +} > > + > > +static int recv_request_devlist(int connfd) { > > + struct op_devlist_request req; > > + int rc; > > + > > + memset(&req, 0, sizeof(req)); > > + > > + rc = usbip_net_recv(connfd, &req, sizeof(req)); > > + if (rc < 0) { > > + dbg("usbip_net_recv failed: devlist request"); > > + return -1; > > + } > > are we waiting here for message of size 0 or I missed something? > > (...) > > 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
