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
signature.asc
Description: OpenPGP digital signature
