From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:53567) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WNH5S-0002ba-4B for qemu-devel@nongnu.org; Tue, 11 Mar 2014 03:26:55 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WNH5Q-000755-JF for qemu-devel@nongnu.org; Tue, 11 Mar 2014 03:26:54 -0400 Received: from mail-wg0-x22a.google.com ([2a00:1450:400c:c00::22a]:44236) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WNH5Q-00074p-7J for qemu-devel@nongnu.org; Tue, 11 Mar 2014 03:26:52 -0400 Received: by mail-wg0-f42.google.com with SMTP id y10so9625499wgg.1 for ; Tue, 11 Mar 2014 00:26:51 -0700 (PDT) MIME-Version: 1.0 Sender: kgrace.liu@gmail.com In-Reply-To: <531E2CC0.8080406@redhat.com> References: <1394436721-21812-1-git-send-email-cyliu@suse.com> <1394436721-21812-4-git-send-email-cyliu@suse.com> <531E20AB.3090409@redhat.com> <531E2CC0.8080406@redhat.com> Date: Tue, 11 Mar 2014 15:26:51 +0800 Message-ID: From: Chunyan Liu Content-Type: multipart/alternative; boundary=047d7b3a8174cb12c504f44fa41c 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: Eric Blake Cc: Kevin Wolf , Dong Xu Wang , qemu-devel@nongnu.org, stefanha@redhat.com --047d7b3a8174cb12c504f44fa41c Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable 2014-03-11 5:21 GMT+08:00 Eric Blake : > On 03/10/2014 02:29 PM, Eric Blake wrote: > > >> + opt =3D qemu_opt_find(opts, name); > >> + if (opt) { > >> + g_free((char *)opt->str); > > > > ...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. > > I've played with this a bit more, and now am more confused. QemuOpts is > a LOT to comprehend. > > Pre-patch, 'qemu-system-x86_64 -nodefaults -machine > type=3Dnone,type-noone' displayed a help message about unknown machine > type "noone", while swapping type=3Dnoone,type=3Dnone proceeded with the > 'none' type. So the last version silently won, which was not the > behavior I had predicted. > > Post-patch, I get a compilation error (so how did you test your patch?): > Mostly tested ./qemu-img commands where QEMUOptionParameter is used. I really didn't think of test QemuOpts fully, and about the test suite, I have no full knowledge about how many things need to be tested, how many things need to be covered? > qapi/opts-visitor.c: In function =E2=80=98opts_start_struct=E2=80=99: > qapi/opts-visitor.c:146:31: error: assignment discards =E2=80=98const=E2= =80=99 qualifier > from pointer target type [-Werror] > ov->fake_id_opt->name =3D "id"; > ^ > This is a place needed to be changed but missing, since name and str changed to be (char *), here should be g_strdup("id"). Due to my configuration, it appears as a warning instead of error which is missed. Updated 3/25. > If I press on in spite of that warning, then I get the same behavior > where the last type=3D still wins on behavior. So I'm not sure how it al= l > worked, but at least behavior wise, my one test didn't uncover a > regression. > > Still, I'd feel a LOT better with a testsuite of what QemuOpts is > supposed to be able to do. tests/test-opts-visitor.c was the only file > in tests/ that even mentions QemuOpts. > > > >> @@ -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. > > Maybe not pointless, if you end up not removing the const in the struct > declaration due to the compile error; but that brings me back to my > earlier question - since the compiler error proves that we have places > that are assigning compile-time string constants into the name field, we > must NOT call g_free on those copies - how does your code distinguish > between a QemuOpt that is built up by mallocs, vs. one that is described > by compile-time constants? > > -- > Eric Blake eblake redhat com +1-919-301-3266 > Libvirt virtualization library http://libvirt.org > > --047d7b3a8174cb12c504f44fa41c Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: quoted-printable



2014-03-11 5:21 GMT+08:00 Eric Blake <eblake@redhat.com>:
On 03/10/2014 02:29 PM, Eric= Blake wrote:

>> + =C2=A0 =C2=A0opt =3D qemu_opt_find(opts, name);
>> + =C2=A0 =C2=A0if (opt) {
>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0g_free((char *)opt->str);
>
> ...which means the cast is pointless here.
>
> Hmm. =C2=A0This means that you are giving opt_set() the behavior of &#= 39;last
> version wins', by silently overwriting earlier versions. =C2=A0If = 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 f= ind the
> FIRST hit. =C2=A0Doesn'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.

I've played with this a bit more, and now am more confused. =C2= =A0QemuOpts is
a LOT to comprehend.

Pre-patch, 'qemu-system-x86_64 -nodefaults -machine
type=3Dnone,type-noone' displayed a help message about unknown machine<= br> type "noone", while swapping type=3Dnoone,type=3Dnone proceeded w= ith the
'none' type. =C2=A0So the last version silently won, which was not = the
behavior I had predicted.

Post-patch, I get a compilation error (so how did you test your patch?):

Mostly tested ./qemu-img commands where QEMUOptionP= arameter is used.
I really didn't think of test QemuOpts = fully, and about the test suite, I have no full
knowledge about how many things need to be tested, how many thin= gs need to be
covered?


qapi/opts-visitor.c: In function =E2=80=98opts_start_struct=E2=80=99:
qapi/opts-visitor.c:146:31: error: assignment discards =E2=80=98const=E2=80= =99 qualifier
from pointer target type [-Werror]
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0ov->fake_id_opt->name =3D "id&= quot;;
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0^

This is a place needed to be changed but missing, since name and str
ch= anged to be (char *), here should be g_strdup("id"). Due to my co= nfiguration,
it appears as a warning instead of error which is missed. Updated 3/25.
=
=C2=A0
If I press on in spite of that warning, then I get the same behavior
where the last type=3D still wins on behavior. =C2=A0So I'm not sure ho= w it all
worked, but at least behavior wise, my one test didn't uncover a regres= sion.

Still, I'd feel a LOT better with a testsuite of what QemuOpts is
supposed to be able to do. =C2=A0tests/test-opts-visitor.c was the only fil= e
in tests/ that even mentions QemuOpts.
=C2=A0
=C2=A0
>> @@ -744,16 +777,24 @@ void qemu_opt_set_err(QemuOpts *opts, const = char *name, const char *value,
>> =C2=A0int qemu_opt_set_bool(QemuOpts *opts, const char *name, bool= val)
>
>> + =C2=A0 =C2=A0opt =3D qemu_opt_find(opts, name);
>> + =C2=A0 =C2=A0if (opt) {
>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0g_free((char *)opt->str);
>
> Another pointless cast.

Maybe not pointless, if you end up not removing the const in the stru= ct
declaration due to the compile error; but that brings me back to my
earlier question - since the compiler error proves that we have places
that are assigning compile-time string constants into the name field, we must NOT call g_free on those copies - how does your code distinguish
between a QemuOpt that is built up by mallocs, vs. one that is described by compile-time constants?

--
Eric Blake =C2=A0 eblake redhat com =C2=A0 =C2=A0+1-919-301-3266
Libvirt virtualization library http://libvirt.org


--047d7b3a8174cb12c504f44fa41c--