From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:52457) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WPmfq-0004Yf-OY for qemu-devel@nongnu.org; Tue, 18 Mar 2014 01:34:52 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WPmfp-0001oF-FM for qemu-devel@nongnu.org; Tue, 18 Mar 2014 01:34:50 -0400 Received: from mail-we0-x235.google.com ([2a00:1450:400c:c03::235]:55256) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WPmfp-0001o9-3d for qemu-devel@nongnu.org; Tue, 18 Mar 2014 01:34:49 -0400 Received: by mail-we0-f181.google.com with SMTP id q58so5326390wes.40 for ; Mon, 17 Mar 2014 22:34:48 -0700 (PDT) MIME-Version: 1.0 Sender: kgrace.liu@gmail.com In-Reply-To: <532055C1.10100@redhat.com> References: <1394436721-21812-1-git-send-email-cyliu@suse.com> <1394436721-21812-6-git-send-email-cyliu@suse.com> <531E4A99.8050306@redhat.com> <531EFA88.8010003@redhat.com> <532055C1.10100@redhat.com> Date: Tue, 18 Mar 2014 13:34:47 +0800 Message-ID: From: Chunyan Liu Content-Type: multipart/alternative; boundary=f46d0442879ef2725d04f4dae446 Subject: Re: [Qemu-devel] [PATCH v22 05/25] add some QemuOpts functions for replace work 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, Stefan Hajnoczi --f46d0442879ef2725d04f4dae446 Content-Type: text/plain; charset=UTF-8 2014-03-12 20:40 GMT+08:00 Eric Blake : > On 03/11/2014 09:10 PM, Chunyan Liu wrote: > > >>>> > >>> Could be if changing qemu_opt_get return value type, but just as said > >>> before, > >>> that will affect many codes. > >> > >> Also, changing an existing function that returns 'const char *' into now > >> returning 'char *' will NOT break any callers if the semantics remain > >> unchanged (what WILL break is if the semantics change where the callers > >> must now free the result) > > > > > > Yes, that's the only change, we need to free the result. There are more > > than 200 > > places using qemu_opt_get in existing code, we need to checkout the > related > > files > > and free the result one by one. > > You can still write your helper function so that if 'del' is true, you > strdup, if 'del' is false, you return the in-place memory. You'll have > to cast away const in the helper, but qemu_opt_get can then restore the > const and you don't have to adjust any existing callers, while still > sharing the underlying implementation rather than duplicating code. After trying, still has blocking here. qemu_opt_get returns either opt->str (this is char *) or desc->def_value_str (this is const char *). If not g_strdup the result, return (const char *) is OK, but return (char *) will has build error when casting (const char *) to (char *). And since assert(opt->desc && opt->desc->type == xx) doesn't need change for this patch series now, qemu_opt_get_bool could still return opt->value.boolean, doesn't need to call qemu_opt_get to get opt->str and parse that to bool. Same to qemu_opt_get_size/number. So, it's not so urgent that qemu_opt_get/qemu_opt_get_del share common helper function. I'm inclined to keep duplicating code. > But > I guess I'll wait for v23 to see what you actually do in response to my > suggestions. If nothing else, please document design decisions in your > commit message (for example, if you choose to duplicate code rather than > share code in a common helper, explain that the duplication is because > one function returns malloc'd memory while the other returns in-place > memory). > > -- > Eric Blake eblake redhat com +1-919-301-3266 > Libvirt virtualization library http://libvirt.org > > --f46d0442879ef2725d04f4dae446 Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: quoted-printable



2014-03-12 20:40 GMT+08:00 Eric Blake <eblake@redhat.com>:
On 03/11/2014 09:10 = PM, Chunyan Liu wrote:

>>>>
>>> Could be if changing qemu_opt_get return value type, but just = as said
>>> before,
>>> that will affect many codes.
>>
>> Also, changing an existing function that returns 'const char *= ' into now
>> returning 'char *' will NOT break any callers if the seman= tics remain
>> unchanged (what WILL break is if the semantics change where the ca= llers
>> must now free the result)
>
>
> Yes, that's the only change, we need to free the result. There are= more
> than 200
> places using qemu_opt_get in existing code, we need to checkout the re= lated
> files
> and free the result one by one.

You can still write your helper function so that if 'del' is = true, you
strdup, if 'del' is false, you return the in-place memory. =C2=A0Yo= u'll have
to cast away const in the helper, but qemu_opt_get can then restore the
const and you don't have to adjust any existing callers, while still sharing the underlying implementation rather than duplicating code. =C2=A0<= /blockquote>

After trying, still has blocking here. qemu= _opt_get returns either opt->str (this
is char *) or desc->def_val= ue_str (this is const char *). If not g_strdup the result,
return (const char *) is OK, but return (char *) will has build = error when casting
(const char *) to (char *).
= =C2=A0
And since assert(opt->desc && opt->desc-= >type =3D=3D xx) doesn't need change
for this patch series now, qemu_opt_get_bool could still return<= br>opt->value.boolean, doesn't need to call qemu_opt_get to get opt-= >str and
parse that to bool. Same to qemu_opt_get_size/number. So, i= t's not so urgent
that qemu_opt_get/qemu_opt_get_del share common helper function. I'm in= clined
to keep duplicating code.
=C2=A0
But
I guess I'll wait for v23 to see what you actually do in response to my=
suggestions. =C2=A0If nothing else, please document design decisions in you= r
commit message (for example, if you choose to duplicate code rather than share code in a common helper, explain that the duplication is because
one function returns malloc'd memory while the other returns in-place memory).

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


--f46d0442879ef2725d04f4dae446--