Re: [PATCH v13 05/10] usbip: exporting devices: modifications to daemon
On 01/04/2017 12:48 AM, fx IWATA NOBUO wrote: > Dear Krzysztof, > > I posted new version and it's improved thanks to your review. I've seen it. It's on my TODO list. I'll get to this in next week (hopefully in the beginning of the week). 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
RE: [PATCH v13 05/10] usbip: exporting devices: modifications to daemon
Dear Krzysztof, I posted new version and it's improved thanks to your review. About introducing usbip_meta_driver_.*(), I decided not to use _meta_. In this series, it becomes an abstraction among daemons so _meta_ is too much for it. usbipd_ prefix has been used so I thought usbipd_driver_.*() is fit to this case. Thank you for your help, nobuo iwata // > -Original Message- > From: Krzysztof Opasiak [mailto:k.opas...@samsung.com] > Sent: Wednesday, December 07, 2016 5:57 PM > To: fx IWATA NOBUO ; > valentina.mane...@gmail.com; shuah...@samsung.com; > gre...@linuxfoundation.org; linux-usb@vger.kernel.org > Cc: fx MICHIMURA TADAO > Subject: Re: [PATCH v13 05/10] usbip: exporting devices: modifications to > daemon > > > > On 12/07/2016 01:05 AM, fx IWATA NOBUO wrote: > > Hello, > > > >> how about usbip_meta_driver_set(type)? > > > > Good. > > > > I'd like to confirm the 'type' parameter. > > > > #define USBIP_DRIVER_TYPE_HOST 0 > > #define USBIP_DRIVER_TYPE_DEVICE 1 > > > > Then usbip_meta_driver_init should not be included. > > It must be usbip_meta_driver_set(USBIP_DRIVER_TYPE_HOST). > > > > Yes, that's exactly what I mean. > > 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
Re: [PATCH v13 05/10] usbip: exporting devices: modifications to daemon
On 12/07/2016 01:05 AM, fx IWATA NOBUO wrote: > Hello, > >> how about usbip_meta_driver_set(type)? > > Good. > > I'd like to confirm the 'type' parameter. > > #define USBIP_DRIVER_TYPE_HOST 0 > #define USBIP_DRIVER_TYPE_DEVICE 1 > > Then usbip_meta_driver_init should not be included. > It must be usbip_meta_driver_set(USBIP_DRIVER_TYPE_HOST). > Yes, that's exactly what I mean. 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
RE: [PATCH v13 05/10] usbip: exporting devices: modifications to daemon
Hello, > how about usbip_meta_driver_set(type)? Good. I'd like to confirm the 'type' parameter. #define USBIP_DRIVER_TYPE_HOST 0 #define USBIP_DRIVER_TYPE_DEVICE 1 Then usbip_meta_driver_init should not be included. It must be usbip_meta_driver_set(USBIP_DRIVER_TYPE_HOST). Thank you, n.iwata // > -Original Message- > From: Krzysztof Opasiak [mailto:k.opas...@samsung.com] > Sent: Tuesday, December 06, 2016 6:51 PM > To: fx IWATA NOBUO; valentina.mane...@gmail.com; shuah...@samsung.com; > gre...@linuxfoundation.org; linux-usb@vger.kernel.org > Cc: fx MICHIMURA TADAO > Subject: Re: [PATCH v13 05/10] usbip: exporting devices: modifications to > daemon > > Hi, > > On 12/06/2016 05:40 AM, fx IWATA NOBUO wrote: > > Hello, > > > >>> usbip_driver_ has widely used as function names and file > >>> names for host side abstraction. > >>> If they were usbip_host_, I can use > >>> usbip_driver_open/close for both host(stub/vudc) and vhci. > >> > >> usbip_host() is not correct name to use for both stub and vudc > >> as from USB point of view stub is on a host but vudc is on a device > >> side > > > > OK. > > > >> maybe a kind of usbipd_backed_init() would be more suitable? > > > > Naming problem again but I recognize usbip_open_driver() is worse than > > connect. > > I think the word 'backend' has wide meaning and more strict word is > > better. > > > > init driver = &host_driver; NA > > ( )driver = &device_driver; NA > > open usbip_driver_open usbip_vhci_driver_open > > close usbip_driver_close usbip_vhci_driver_close > > > > Here, I'd like to use word 'driver'. > > I searched analogy meta_, super_ in kernel. > > > > How about usbip_meta_driver_init/open/close()? > > Sounds good. Let's try. > > > > >>>> usbip_update_driver() is t totally unrelated to what this > >>>> assignment really does. > >> So as above. I suggest to call it usbipd_backend() instead of driver. > > > > Please, let me know good verb representing 'driver = &device_driver'. > > how about usbip_meta_driver_set(type)? > > 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
Re: [PATCH v13 05/10] usbip: exporting devices: modifications to daemon
Hi, On 12/06/2016 05:40 AM, fx IWATA NOBUO wrote: > Hello, > >>> usbip_driver_ has widely used as function names and file >>> names for host side abstraction. >>> If they were usbip_host_, I can use usbip_driver_open/close >>> for both host(stub/vudc) and vhci. >> >> usbip_host() is not correct name to use for both stub and vudc as from >> USB point of view stub is on a host but vudc is on a device side > > OK. > >> maybe a kind of usbipd_backed_init() would be more suitable? > > Naming problem again but I recognize usbip_open_driver() is worse than > connect. > I think the word 'backend' has wide meaning and more strict word is > better. > > init driver = &host_driver; NA > ( )driver = &device_driver; NA > open usbip_driver_open usbip_vhci_driver_open > close usbip_driver_close usbip_vhci_driver_close > > Here, I'd like to use word 'driver'. > I searched analogy meta_, super_ in kernel. > > How about usbip_meta_driver_init/open/close()? Sounds good. Let's try. > usbip_update_driver() is t totally unrelated to what this assignment really does. >> So as above. I suggest to call it usbipd_backend() instead of driver. > > Please, let me know good verb representing 'driver = &device_driver'. how about usbip_meta_driver_set(type)? 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
RE: [PATCH v13 05/10] usbip: exporting devices: modifications to daemon
Hello, > > usbip_driver_ has widely used as function names and file > > names for host side abstraction. > > If they were usbip_host_, I can use usbip_driver_open/close > > for both host(stub/vudc) and vhci. > > usbip_host() is not correct name to use for both stub and vudc as from > USB point of view stub is on a host but vudc is on a device side OK. > maybe a kind of usbipd_backed_init() would be more suitable? Naming problem again but I recognize usbip_open_driver() is worse than connect. I think the word 'backend' has wide meaning and more strict word is better. init driver = &host_driver; NA ( )driver = &device_driver; NA open usbip_driver_open usbip_vhci_driver_open close usbip_driver_close usbip_vhci_driver_close Here, I'd like to use word 'driver'. I searched analogy meta_, super_ in kernel. How about usbip_meta_driver_init/open/close()? > >> usbip_update_driver() is t totally unrelated to what this assignment > >> really does. > So as above. I suggest to call it usbipd_backend() instead of driver. Please, let me know good verb representing 'driver = &device_driver'. > >>> + 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. I found the 'reply' is not used so I will remove the definition too. > > PACK_OP_PACK 1 > > PACK_OP_UNPACK 0 > > Please do this outside this set as it's totally unrelated and just > mark this series as depending on that change. It's a good idea to > change this in all files not only in those you touch. OK. I will. Thank you for your help, n.iwata // > -Original Message- > From: Krzysztof Opasiak [mailto:k.opas...@samsung.com] > Sent: Friday, December 02, 2016 11:20 PM > To: fx IWATA NOBUO; valentina.mane...@gmail.com; shuah...@samsung.com; > gre...@linuxfoundation.org; linux-usb@vger.kernel.org > Cc: fx MICHIMURA TADAO > Subject: Re: [PATCH v13 05/10] usbip: exporting devices: modifications to > daemon > > > > On 12/02/2016 09:36 AM, fx IWATA NOBUO wrote: > > 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_ has widely used as function names and file > > names for host side abstraction. > > If they were usbip_host_, I can use usbip_driver_open/close > > for both host(stub/vudc) and vhci. > > usbip_host() is not correct name to use for both stub and vudc as from > USB point of view stub is on a host but vudc is on a device side > > maybe a kind of usbipd_backed_init() would be more suitable? > > > > >> 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. > > > > So as above. I suggest to call it usbipd_backend() instead of driver. > > >> 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. > > It's misleading as in USB we have
Re: [PATCH v13 05/10] usbip: exporting devices: modifications to daemon
On 12/02/2016 09:36 AM, fx IWATA NOBUO wrote: > 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_ has widely used as function names and file names > for host side abstraction. > If they were usbip_host_, I can use usbip_driver_open/close > for both host(stub/vudc) and vhci. usbip_host() is not correct name to use for both stub and vudc as from USB point of view stub is on a host but vudc is on a device side maybe a kind of usbipd_backed_init() would be more suitable? > >> 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. > So as above. I suggest to call it usbipd_backend() instead of driver. >> 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. It's misleading as in USB we have host and device and this file is used on both (stub and vudc). > >>> + 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 > Please do this outside this set as it's totally unrelated and just mark this series as depending on that change. It's a good idea to change this in all files not only in those you touch. 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
RE: [PATCH v13 05/10] usbip: exporting devices: modifications to daemon
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_ has widely used as function names and file names
for host side abstraction.
If they were usbip_host_, 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.opas...@samsung.com]
> Sent: Friday, December 02, 2016 6:19 AM
> To: fx IWATA NOBUO; valentina.mane...@gmail.com; shuah...@samsung.com;
> gre...@linuxfoundation.org; linux-usb@vger.kernel.org
> 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",
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;
>
