From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:47066) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WN6pQ-0000YW-2y for qemu-devel@nongnu.org; Mon, 10 Mar 2014 16:29:44 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WN6pL-0003vV-Bj for qemu-devel@nongnu.org; Mon, 10 Mar 2014 16:29:40 -0400 Received: from mx1.redhat.com ([209.132.183.28]:63532) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WN6pL-0003uf-4e for qemu-devel@nongnu.org; Mon, 10 Mar 2014 16:29:35 -0400 Message-ID: <531E20AB.3090409@redhat.com> Date: Mon, 10 Mar 2014 14:29:31 -0600 From: Eric Blake MIME-Version: 1.0 References: <1394436721-21812-1-git-send-email-cyliu@suse.com> <1394436721-21812-4-git-send-email-cyliu@suse.com> In-Reply-To: <1394436721-21812-4-git-send-email-cyliu@suse.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="8CCwrFtQgELMpU0Sa4BunGsXidhHfr5hV" Subject: Re: [Qemu-devel] [PATCH v22 03/25] improve some functions in qemu-option.c List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Chunyan Liu , qemu-devel@nongnu.org Cc: kwolf@redhat.com, Dong Xu Wang , stefanha@redhat.com This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --8CCwrFtQgELMpU0Sa4BunGsXidhHfr5hV Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable On 03/10/2014 01:31 AM, Chunyan Liu wrote: > Improve opt_get and opt_set group of functions. For opt_get, check and = handle > NULL input; for opt_set, when set to an existing option, rewrite the op= tion > with new value. >=20 > Signed-off-by: Dong Xu Wang > Signed-off-by: Chunyan Liu > --- > include/qemu/option_int.h | 4 +-- > util/qemu-option.c | 81 +++++++++++++++++++++++++++++++++++++++= -------- > 2 files changed, 69 insertions(+), 16 deletions(-) >=20 >=20 > +++ b/include/qemu/option_int.h > @@ -30,8 +30,8 @@ > #include "qemu/error-report.h" > =20 > struct QemuOpt { > - const char *name; > - const char *str; > + char *name; > + char *str; You are losing the 'const' here... > @@ -704,6 +730,13 @@ static void opt_set(QemuOpts *opts, const char *na= me, const char *value, > return; > } > =20 > + opt =3D qemu_opt_find(opts, name); > + if (opt) { > + g_free((char *)opt->str); =2E..which means the cast is pointless here. Hmm. This means that you are giving opt_set() the behavior of 'last version wins', by silently overwriting earlier versions. If I'm understanding the existing code correctly, the previous behavior was that calling opt_set twice in a row on the same name would inject BOTH names into 'opts', but then subsequent lookups on opts would find the FIRST hit. Doesn't that mean this is a semantic change: qemu -opt key=3Dvalue1,key=3Dvalue2 would previously set key to value1, but now sets key to value2. Is it intentional? If so, document it in the commit message. Or am I missing something, where we already reject duplicates, in which case, your new code is currently unreachable, and your commit message could do with more explanation of why you are adding something that later patches will make use of. Also, I'd feel a LOT better if we FIRST had a testsuite that covered what QemuOpts is _supposed_ to do before we then patch it,so that we aren't getting surprised by silent changes in behavior. It's okay to write a test that shows old behavior, then tweak the test in the patch that updates the behavior along with the justification of why the new behavior is nicer. > @@ -744,16 +777,24 @@ void qemu_opt_set_err(QemuOpts *opts, const char = *name, const char *value, > int qemu_opt_set_bool(QemuOpts *opts, const char *name, bool val) > + opt =3D qemu_opt_find(opts, name); > + if (opt) { > + g_free((char *)opt->str); Another pointless cast. > =20 > + opt =3D qemu_opt_find(opts, name); > + if (opt) { > + g_free((char *)opt->str); and another. --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --8CCwrFtQgELMpU0Sa4BunGsXidhHfr5hV 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/ iQEcBAEBCAAGBQJTHiCrAAoJEKeha0olJ0NqyWwH/jbdvMe222QYgjGr4cGU31Zo sVIOVi0nomkVhwHA0/u5NUrHoiF6BmOiez2ImOaEMaga+Ml4aaXJNFw9epId0u43 K5tBOKPi+5n20Cii+/9Bn34KtAfoG3LrXjTeWOBaIQNwgkFziSiw4fcUA90BrCtD 65iZJeuFYCusG5y2rN8dnumJDnIIo+ohyDeRg6LDcFELJtrfrzqyrEs5qALjthgQ lULAvtbZ3Jsq0fbZLuHCV1LCJHSWK73MtQvwVopozUA3LakjRvS1byBOTFh5O/7l GvffHPUfRllRyMLs8eek7BZM7E6FEfd5N+yK6xhABPWck2sNdNbsi56CsHzT0Ek= =of/R -----END PGP SIGNATURE----- --8CCwrFtQgELMpU0Sa4BunGsXidhHfr5hV--