From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:33099) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Wu7xr-0001cc-Eq for qemu-devel@nongnu.org; Mon, 09 Jun 2014 18:22:56 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Wu7xk-0000To-RY for qemu-devel@nongnu.org; Mon, 09 Jun 2014 18:22:51 -0400 Received: from mx1.redhat.com ([209.132.183.28]:43894) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Wu7xk-0000TL-Ib for qemu-devel@nongnu.org; Mon, 09 Jun 2014 18:22:44 -0400 Message-ID: <539633B0.1070908@redhat.com> Date: Mon, 09 Jun 2014 16:22:40 -0600 From: Eric Blake MIME-Version: 1.0 References: <20140527120050.15172.94908.stgit@3820> <20140527120638.15172.80806.stgit@3820> <539095F0.5020504@redhat.com> In-Reply-To: Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="x5hVR8EnXxfoBlBl95KAUBNAkw18Rv212" Subject: Re: [Qemu-devel] [snabb-devel] Re: [PATCH v10 15/18] Add the vhost-user netdev backend to the command line List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Nikolay Nikolaev , "snabb-devel@googlegroups.com" Cc: Antonios Motakis , Luke Gorrie , VirtualOpenSystems Technical Team , qemu-devel , mst This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --x5hVR8EnXxfoBlBl95KAUBNAkw18Rv212 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable On 06/09/2014 03:19 PM, Nikolay Nikolaev wrote: >>> +static CharDriverState *net_vhost_parse_chardev( >>> + const NetdevVhostUserOptions *opts) >> >> Unusual indentation, but I see your dilemma of fitting 80 columns. >> > What woudl be the right approach here, is leaving it 84 colums acceptab= le: checkpatch.pl is for guidance; it is okay to have a patch that doesn't pass. At any rate, I won't reject a patch for long lines, or for unusual style, where it is obvious that there is no way to fit things into 80 columns while still complying with all other style issues. >>> + error_report("vhost-user requires frontend driver >> virtio-net-*"); >> >> How many of these error_report() should be using Error **errp logistic= s >> instead? >> >> I don't see this 'errp' logic applied here. These are static functions= > called in the init function to check for compatible command line parame= ters. > This init fucntion is part of 'net_client_init_fun[]" which does not > provide 'Error **errp' argument. The way I see it, there is no way to > propagate the 'errp' up. Or am I getting this wrong? It was more of a 'food for thought' question - we may eventually want to enhance the code base to be able to propagate errp up; but since that is a more invasive patch, and your series isn't making the situation worse, it does not necessarily mean you have to do that additional work. On the other hand, if the conversion is worth doing, then getting it done sooner makes it easier to take advantage of the improved error handling for other new code in the same area. >>> + >>> + /* verify net frontend */ >>> + if (qemu_opts_foreach(qemu_find_opts("device"), net_vhost_check_= net, >>> + (void *)name, true) =3D=3D -1) { >> >> This is C; why do you need a cast to (void*)? >> > Because of the constness in 'const char *name'. I am casting to 'char *= > now', is it better? Okay, I see. Sometimes, I like to put in a comment /* cast away const */ to make it obvious why I'm doing things like that. It also serves as a spur to investigate a couple of things: 1) should the callback signature should have allowed a const in the first place, rather than making callers have to cast away const, 2) if the callback can modify the pointer but I'm casting away const, am I going to cause data corruption and/or a crash --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --x5hVR8EnXxfoBlBl95KAUBNAkw18Rv212 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 Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQEcBAEBCAAGBQJTljOwAAoJEKeha0olJ0NqF3oH/iVjNaJDBganV4/mVE4cCbo/ C8f0r9rl5rTGNTin0LNPwDFGXVgQ1NgXHqIwuTGRsITirLvjP6aFmGG0T6/zrXQD 51FjmeZ9cjkZmjc37QDD8BpT9WrXfZa2tzm8jmKKZ3GjONPMcsCNRCpDczCBdWbA MG285TheZtgqqhMb2emBgLAXrgbsKvzaTbFU7fD5CwaCLxFCg4PUvWWrB+y7hgtv gBefZoSwDRvXLhAVCjfSpgDljcmmMkFV34EzLpLxZic5YJwUb9NKXJgN9DUMqFYG DgstYzwjn/WizUDTLXC9DKU4VXCJKu6sGhiKAt8rEOEeDqYRXAKmeCZh6ifu2LU= =ySog -----END PGP SIGNATURE----- --x5hVR8EnXxfoBlBl95KAUBNAkw18Rv212--