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
