From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:40735) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1br8GO-0004Gm-19 for qemu-devel@nongnu.org; Mon, 03 Oct 2016 14:47:00 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1br8GL-0000us-PO for qemu-devel@nongnu.org; Mon, 03 Oct 2016 14:46:54 -0400 References: <20160928205602.17275-1-mreitz@redhat.com> <20160928205602.17275-6-mreitz@redhat.com> From: Eric Blake Message-ID: <281c5a1d-d7a1-bfca-27b6-33fac236e217@redhat.com> Date: Mon, 3 Oct 2016 13:46:40 -0500 MIME-Version: 1.0 In-Reply-To: <20160928205602.17275-6-mreitz@redhat.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="peL0hscOpnrbClBsq5eo6MjLOTeKCncFD" Subject: Re: [Qemu-devel] [PATCH v4 05/12] block/nbd: Add nbd_has_filename_options_conflict() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Max Reitz , qemu-block@nongnu.org Cc: qemu-devel@nongnu.org, Kevin Wolf , Paolo Bonzini , Markus Armbruster , "Daniel P. Berrange" This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --peL0hscOpnrbClBsq5eo6MjLOTeKCncFD From: Eric Blake To: Max Reitz , qemu-block@nongnu.org Cc: qemu-devel@nongnu.org, Kevin Wolf , Paolo Bonzini , Markus Armbruster , "Daniel P. Berrange" Message-ID: <281c5a1d-d7a1-bfca-27b6-33fac236e217@redhat.com> Subject: Re: [PATCH v4 05/12] block/nbd: Add nbd_has_filename_options_conflict() References: <20160928205602.17275-1-mreitz@redhat.com> <20160928205602.17275-6-mreitz@redhat.com> In-Reply-To: <20160928205602.17275-6-mreitz@redhat.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 09/28/2016 03:55 PM, Max Reitz wrote: > Right now, we have four possible options that conflict with specifying > an NBD filename, and a future patch will add another one ("address"). > This future option is a nested QDict that is flattened at this point, > requiring us to test each option whether its key has an "address." > prefix. Therefore, we will then need to iterate through all options. How does the plans to add 'address.' interact with Dan's patches for auto-nesting when parsing command line options? https://lists.gnu.org/archive/html/qemu-devel/2016-09/msg08238.html I'm wondering if your patches can be made a bit smaller by basing on top of that work (allowing the command-line user to omit 'address.' and the parser auto-nests as a result, while the QMP form is always nested). >=20 > Adding this iteration logic now will simplify adding the new option > later. A nice side effect is that the user will not receive a long list= > of five options which are not supposed to be specified with a filename,= > but we can actually print the problematic option. On its own, this patch looks reasonable, but I'm a bit worried that with all the other parsing changes going in, it may result in unused code. >=20 > Signed-off-by: Max Reitz > --- > block/nbd.c | 26 ++++++++++++++++++++------ > 1 file changed, 20 insertions(+), 6 deletions(-) >=20 > diff --git a/block/nbd.c b/block/nbd.c > index c539fb5..cdab20f 100644 > --- a/block/nbd.c > +++ b/block/nbd.c > @@ -123,6 +123,25 @@ out: > return ret; > } > =20 > +static bool nbd_has_filename_options_conflict(QDict *options, Error **= errp) > +{ > + const QDictEntry *e; > + > + for (e =3D qdict_first(options); e; e =3D qdict_next(options, e)) = { > + if (!strcmp(e->key, "host") || > + !strcmp(e->key, "port") || > + !strcmp(e->key, "path") || > + !strcmp(e->key, "export")) > + { > + error_setg(errp, "Option '%s' cannot be used with a file n= ame", > + e->key); > + return true; > + } > + } Or put another way, while manual parsing looks fine, it's even better if we can avoid manual parsing (and having to remember to update it when the schema changes) by letting the schema itself automatically reject invalid option pairs, even if the automatic rejection doesn't give quite as nice of an error message. > + > + return false; > +} > + > static void nbd_parse_filename(const char *filename, QDict *options, > Error **errp) > { > @@ -131,12 +150,7 @@ static void nbd_parse_filename(const char *filenam= e, QDict *options, > const char *host_spec; > const char *unixpath; > =20 > - if (qdict_haskey(options, "host") > - || qdict_haskey(options, "port") > - || qdict_haskey(options, "path")) > - { Also, this patch looks like you are doing a bug fix mixed with code motion - the old code manually checked for duplicates on only three options, but the new code adds 'export' to the mix. > - error_setg(errp, "host/port/path and a file name may not be sp= ecified " > - "at the same time"); > + if (nbd_has_filename_options_conflict(options, errp)) { > return; > } > =20 >=20 --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --peL0hscOpnrbClBsq5eo6MjLOTeKCncFD Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 Comment: Public key at http://people.redhat.com/eblake/eblake.gpg Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQEcBAEBCAAGBQJX8qeQAAoJEKeha0olJ0NqzsgH/iuNOu1LFjV2bl6wT0LeClHZ dBTx3/huQ2s9FTCecQcISYYxIKKAj3mfVwaWaLknYvjHn6p//q3mpNu1LBZEp03/ F8o/GTRNqdU+tKiZ5iRwWZHSJ+9mq5vtyyQXjjG91nxxO2vUbQmlY9scBpu9dXsK mL04idnsDi7x5kEXm8OARStZMHPyF1we7EExmEHFfYjOZBk7nTuoP1CJXH+aBb4X /6KrocOa0nGEO8XuE1YwtD+5z0VpgrT7QSZg9uQOauyDYrG8GbdE8pHKVdPWzKff Ipb6yo3yY9W9sSkcpjDkpeXZ2KEHUpHF64JDIZ6Acw26M7riC19aRPdYQLi7zco= =I1NN -----END PGP SIGNATURE----- --peL0hscOpnrbClBsq5eo6MjLOTeKCncFD--