Re: [Qemu-devel] [PATCH] inetd enabled qemu-nbd

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 4639 bytes --]

On 10/28/2014 09:36 PM, Jun Sheng wrote:
> From: Chaos Eternal <chaos@shlug.org>
> 
> 
> Signed-off-by: Jun Sheng <chaoseternal@gmail.com>

Using a pseudonym for the git authorship (the From: line) is unusual
(but not unheard of in this project).  What is weirder is that your
S-o-B uses a real name; if you don't mind exposing your real name, then
why not use it everywhere?

Your commit is missing the body that you sent in the previous message
(<1414553785-23571-1-git-send-email-chaoseternal@gmail.com>).

When sending an updated version of a patch, use 'git send-email -v2' to
include a v2 in the subject line.  Also, it is better to send your
revision as a brand new thread rather than buried in-reply-to an
existing thread.

> ---
>  qemu-nbd.c | 42 ++++++++++++++++++++++++++++++++++--------
>  1 file changed, 34 insertions(+), 8 deletions(-)
> 
> diff --git a/qemu-nbd.c b/qemu-nbd.c
> index b524b34..44bc349 100644
> --- a/qemu-nbd.c
> +++ b/qemu-nbd.c
> @@ -49,6 +49,7 @@ static NBDExport *exp;
>  static int verbose;
>  static char *srcpath;
>  static char *sockpath;
> +static int inetd_fd = -1;
>  static int persistent = 0;
>  static enum { RUNNING, TERMINATE, TERMINATING, TERMINATED } state;
>  static int shared = 1;
> @@ -66,6 +67,7 @@ static void usage(const char *name)
>  "Connection properties:\n"
>  "  -p, --port=PORT           port to listen on (default `%d')\n"
>  "  -b, --bind=IFACE          interface to bind to (default `0.0.0.0')\n"
> +"  -i, --inetd=FD            run as an inetd services on FD\n"

s/services/service/

>  "  -k, --socket=PATH         path to the unix socket\n"
>  "                            (default '"SOCKET_PATH"')\n"
>  "  -e, --shared=NUM          device can be shared by NUM clients (default '1')\n"
> @@ -363,7 +365,13 @@ static void nbd_accept(void *opaque)
>      struct sockaddr_in addr;
>      socklen_t addr_len = sizeof(addr);
>  
> -    int fd = accept(server_fd, (struct sockaddr *)&addr, &addr_len);
> +    int fd ;

No space before ';'

> +    if (inetd_fd < 0) {
> +        fd = accept(server_fd, (struct sockaddr *)&addr, &addr_len);
> +    } else {
> +      fd = server_fd;

Indentation is off.

> +    }
> +
>      if (fd < 0) {
>          perror("accept");
>          return;
> @@ -395,10 +403,11 @@ int main(int argc, char **argv)
>      off_t fd_size;
>      QemuOpts *sn_opts = NULL;
>      const char *sn_id_or_name = NULL;
> -    const char *sopt = "hVb:o:p:rsnP:c:dvk:e:f:tl:";
> +    const char *sopt = "hVi:b:o:p:rsnP:c:dvk:e:f:tl:";

Here, you list 'i' before 'b'...

>      struct option lopt[] = {
>          { "help", 0, NULL, 'h' },
>          { "version", 0, NULL, 'V' },
> +        { "inetd", 1, NULL, 'i'},
>          { "bind", 1, NULL, 'b' },

...here too...

>          { "port", 1, NULL, 'p' },
>          { "socket", 1, NULL, 'k' },
> @@ -510,6 +519,16 @@ int main(int argc, char **argv)
>          case 'b':
>              bindto = optarg;
>              break;
> +        case 'i':

...but here you listed in a different order.  I'm a fan of having the
getopt string in the same order as the case statement (a truly OCD
reviewer would want the case statement and therefore the getopt string
alphabetized, maybe allowing for case insensitive sorting - but that
would be a separate cleanup and is not something I demand).

> +            inetd_fd = strtol(optarg, &end, 10);

Improper use of strtol.  You didn't check for errors via errno.  Rather
than open-coding the correct use of strtol (which is surprisingly
difficult for people to do), you should instead reuse one of our
wrappers that already does the job (for example, util/cutils.c includes
qemu_parse_fd that sounds exactly like what you want).

> @@ -752,9 +775,12 @@ int main(int argc, char **argv)
>          /* Shut up GCC warnings.  */
>          memset(&client_thread, 0, sizeof(client_thread));
>      }
> -
> -    qemu_set_fd_handler2(fd, nbd_can_accept, nbd_accept, NULL,
> -                         (void *)(uintptr_t)fd);
> +    if (inetd_fd < 0) {
> +        qemu_set_fd_handler2(fd, nbd_can_accept, nbd_accept, NULL,
> +                             (void *)(uintptr_t)fd);
> +    } else {
> +        nbd_accept((void *) (uintptr_t) fd);

Inconsistent spacing between the two examples of double casting.  (Alas,
this is one place where the compiler balks if you don't have the double
casting, even though C does not strictly require it).

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 539 bytes --]