From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:49643) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Xh2pQ-0001O0-CM for qemu-devel@nongnu.org; Wed, 22 Oct 2014 16:48:24 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Xh2pM-0004Yy-2P for qemu-devel@nongnu.org; Wed, 22 Oct 2014 16:48:20 -0400 Received: from mx1.redhat.com ([209.132.183.28]:33044) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Xh2pL-0004YZ-Rc for qemu-devel@nongnu.org; Wed, 22 Oct 2014 16:48:16 -0400 Message-ID: <5448180A.8060403@redhat.com> Date: Wed, 22 Oct 2014 14:48:10 -0600 From: Eric Blake MIME-Version: 1.0 References: <1414004099-13209-1-git-send-email-chaoseternal@gmail.com> In-Reply-To: <1414004099-13209-1-git-send-email-chaoseternal@gmail.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="3kujc5wh2vlAIa2nEtSBJx7vKlwBoPJaX" Subject: Re: [Qemu-devel] A Patch to enable qemu-nbd run as an inetd service List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Jun Sheng , qemu-devel@nongnu.org Cc: Chaos Eternal This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --3kujc5wh2vlAIa2nEtSBJx7vKlwBoPJaX Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 10/22/2014 12:54 PM, Jun Sheng wrote: > From: Chaos Eternal >=20 >=20 > 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 >=20 > @@ -363,7 +365,13 @@ static void nbd_accept(void *opaque) > struct sockaddr_in addr; > socklen_t addr_len =3D sizeof(addr); > =20 > - int fd =3D accept(server_fd, (struct sockaddr *)&addr, &addr_len);= > + =20 > + int fd ; > + if (use_inetd =3D=3D 0)=20 > + fd =3D accept(server_fd, (struct sockaddr *)&addr, &addr_len); > + else > + fd =3D 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[] =3D { > { "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 =3D -1; > int ret; > int fd; > + int inet_fd =3D 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 =3D false; > bool seen_discard =3D false; > #ifdef CONFIG_LINUX_AIO > @@ -510,6 +520,16 @@ int main(int argc, char **argv) > case 'b': > bindto =3D optarg; > break; > + case 'i': > + use_inetd =3D 1; Prefer bool (with true/false values) over int (with 0/1 values). Or even better, use inet_fd=3D=3D-1 as the witness of no inetd parameter, an= d a non-negative value as witness of a user-supplied fd, and then you don't need 'use_inetd' at all. > + inet_fd =3D 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 <=3D 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 =3D=3D 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. --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --3kujc5wh2vlAIa2nEtSBJx7vKlwBoPJaX Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 Comment: Public key at http://people.redhat.com/eblake/eblake.gpg iQEcBAEBCAAGBQJUSBgKAAoJEKeha0olJ0NqsBUH/3yOtC1XBVmQZ0idfkhdfEBS KaAzuRqTjjkrzo0ohXSyGM55Ip3zM7fYJIR7ebxx81Qpv4CCO5lTH2s9Wb2xc7ms Pw3MHG+hZzjV/hdSCFy7UFkx7soE34dTkUnL+oA5ZgZHo/dA+qCXAywsp/NiabOu /iwfh46+tgSLNSE1CvhM27sgS3QIp3Tbv23NyFy97YHuqXxIUZrkdPrVp9+DST8q 2+mZoAAVWVM+2/SKwjVN+VBkcsS5tQ86b1aLYFKr44FW9Geb5V+O3s8G7bqL5nuW llBcCBbLbLe5uPxdPS7ILGLEofE++ho0bDCqak7rA9KzmmY3hTN0PhYWn+FLFhM= =vA7Y -----END PGP SIGNATURE----- --3kujc5wh2vlAIa2nEtSBJx7vKlwBoPJaX--