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

[Jun Sheng]

Hi,

Since errx is used everywhere in qemu-nbd.c, I suggest that I follow
this convention in submitting my patch.

On Thu, Oct 23, 2014 at 4:48 AM, Eric Blake <address@hidden> wrote:
> On 10/22/2014 12:54 PM, Jun Sheng wrote:
>> From: Chaos Eternal <address@hidden>
>>
>>
>> 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
>