Re: [Qemu-devel] A Patch to enable qemu-nbd run as an inetd service

[Eric Blake <eblake@redhat.com>]

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

On 10/22/2014 12:54 PM, Jun Sheng wrote:
> From: Chaos Eternal <chaos@shlug.org>
> 
> 
> run qemu-nbd as an inetd service has some benefits
> * more scriptable, such as serve multiple images to different clients
> on one ip/port
> * access control using tcpd
> 

> @@ -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 ;
> +    if (use_inetd == 0) 
> +      fd = accept(server_fd, (struct sockaddr *)&addr, &addr_len);
> +    else
> +      fd = server_fd;

Please run ./scripts/checkpatch.pl on your submission.  It would have
pointed out that our coding style requires {} on both branches of
if/else statements, indentation by 4-space multiples, as well as no
space before semicolon.  This is documented on
http://wiki.qemu.org/Contribute/SubmitAPatch
:";
>      struct option lopt[] = {
>          { "help", 0, NULL, 'h' },
>          { "version", 0, NULL, 'V' },
> +	{ "inetd", 1, NULL, 'i'},

TAB damage. Also would have been caught by checkpatch.pl

>          { "bind", 1, NULL, 'b' },
>          { "port", 1, NULL, 'p' },
>          { "socket", 1, NULL, 'k' },
> @@ -430,6 +439,7 @@ int main(int argc, char **argv)
>      int partition = -1;
>      int ret;
>      int fd;
> +    int inet_fd = 10;

Magic number.  Also, why do you even need to pre-initialize it?  But
pre-initializing fds to -1 feels safer than to a random value that may
or may not be a valid fd.

>      bool seen_cache = false;
>      bool seen_discard = false;
>  #ifdef CONFIG_LINUX_AIO
> @@ -510,6 +520,16 @@ int main(int argc, char **argv)
>          case 'b':
>              bindto = optarg;
>              break;
> +	case 'i':
> +  	    use_inetd = 1;

Prefer bool (with true/false values) over int (with 0/1 values).  Or
even better, use inet_fd==-1 as the witness of no inetd parameter, and a
non-negative value as witness of a user-supplied fd, and then you don't
need 'use_inetd' at all.

> +	    inet_fd = strtol(optarg, &end, 0);
> +	    if (*end) {
> +                errx(EXIT_FAILURE, "Invalid inet fd `%s'", optarg);

Improper use of strtol.  You are correctly rejecting "0a" as garbage,
but fail to detect overflow like "99999999999999999" or the empty string
"" as garbage.

> +            }
> +            if (inet_fd < 3 || inet_fd > 65535) {

Magic numbers.  I can understand why you are requiring an fd larger than
STDERR_FILENO (but spell it 'inet_fd <= STDERR_FILENO', not 'inet_fd <
3); but arbitrarily capping at 64k is bogus.  Better to just try and use
the fd, and it either works,

> +                errx(EXIT_FAILURE, "Inet fd out of range `%s', should be in [3, 65535]", optarg);

errx() is non-standard; qemu-nbd.c is the only file that uses it, but it
would be a nice cleanup (as a separate patch) to convert over to more
idiomatic error reporting similar to the rest of the qemu code base.

> @@ -752,9 +776,11 @@ 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,
> +    if (use_inetd == 0)
> +      qemu_set_fd_handler2(fd, nbd_can_accept, nbd_accept, NULL,
>                           (void *)(uintptr_t)fd);

Indentation of the second line needs to be modified when moving the
first line.  Same comments as earlier about {} and 4-space indentation.

-- 
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 --]