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 <ebl...@redhat.com> wrote:
> On 10/22/2014 12:54 PM, Jun Sheng wrote:
>> From: Chaos Eternal <ch...@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
>
