From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:53063) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WFePC-00017x-A8 for qemu-devel@nongnu.org; Tue, 18 Feb 2014 01:43:47 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WFePB-0005Kg-3T for qemu-devel@nongnu.org; Tue, 18 Feb 2014 01:43:46 -0500 Received: from mail-ve0-x22c.google.com ([2607:f8b0:400c:c01::22c]:43441) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WFePA-0005Ka-Th for qemu-devel@nongnu.org; Tue, 18 Feb 2014 01:43:45 -0500 Received: by mail-ve0-f172.google.com with SMTP id c14so13008673vea.17 for ; Mon, 17 Feb 2014 22:43:44 -0800 (PST) MIME-Version: 1.0 Sender: kgrace.liu@gmail.com In-Reply-To: <52FC043F.1090706@redhat.com> References: <1392186806-10418-1-git-send-email-cyliu@suse.com> <1392186806-10418-5-git-send-email-cyliu@suse.com> <52FC043F.1090706@redhat.com> Date: Tue, 18 Feb 2014 14:43:44 +0800 Message-ID: From: Chunyan Liu Content-Type: multipart/alternative; boundary=089e0158a82aee49c004f2a897d5 Subject: Re: [Qemu-devel] [PATCH v20 04/26] 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, stefanha@redhat.com --089e0158a82aee49c004f2a897d5 Content-Type: text/plain; charset=UTF-8 2014-02-13 7:31 GMT+08:00 Eric Blake : > On 02/11/2014 11:33 PM, Chunyan Liu wrote: > > Add some qemu_opt functions to replace the same functionality of > > QEMUOptionParameter handling. > > > > Signed-off-by: Dong Xu Wang > > Signed-off-by: Chunyan Liu > > --- > > include/qemu/option.h | 9 +++ > > util/qemu-option.c | 134 > +++++++++++++++++++++++++++++++++++++++++++++++++ > > 2 files changed, 143 insertions(+), 0 deletions(-) > > > > > +static void qemu_opt_del(QemuOpt *opt); > > + > > +const char *qemu_opt_get_del(QemuOpts *opts, const char *name) > > +{ > > + const char *str = qemu_opt_get(opts, name); > > + QemuOpt *opt = qemu_opt_find(opts, name); > > + if (opt) { > > + qemu_opt_del(opt); > > Is it any more efficient to reduce the number of times you crawl through > the list? For example, instead of crawling the list with qemu_opt_get, > why not get teh string directly from the result of qemu_opt_find? Also, > you crawl again for qemu_opt_del. Maybe it's unavoidable, and certainly > still O(n), so not the end of the world, but worth thinking about. > In previous Dong Xu's patch, it's commented that there is too much code duplication in qemu_opt_get and qemu_opt_get_del, could using qemu_opt_get directly. Well, I'll improve that. Add "remove" flag to _qemu_opt_get to handle delete or not, and then wrapper _qemu_opt_get to generate qemu_opt_get and qemu_opt_get_del. > +void qemu_opts_print_help(QemuOptsList *list) > > +{ > > + int i; > > + printf("Supported options:\n"); > > Why printf? Might there be callers that want to print to somewhere > other than stdout? > Just followed print_option_help. Existing print_option_help uses printf. Could improve of course. > > -- > Eric Blake eblake redhat com +1-919-301-3266 > Libvirt virtualization library http://libvirt.org > > --089e0158a82aee49c004f2a897d5 Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: quoted-printable



2014-02-13 7:31 GMT+08:00 Eric Blake <eblake@redhat.com>:
On 02/11/= 2014 11:33 PM, Chunyan Liu wrote:
> Add some qemu_opt functions to replace the same functionality of
> QEMUOptionParameter handling.
>
> Signed-off-by: Dong Xu Wang <wdongxu@linux.vnet.ibm.com>
> Signed-off-by: Chunyan Liu <cyliu= @suse.com>
> ---
> =C2=A0include/qemu/option.h | =C2=A0 =C2=A09 +++
> =C2=A0util/qemu-option.c =C2=A0 =C2=A0| =C2=A0134 ++++++++++++++++++++= +++++++++++++++++++++++++++++
> =C2=A02 files changed, 143 insertions(+), 0 deletions(-)
>

> +static void qemu_opt_del(QemuOpt *opt);
> +
> +const char *qemu_opt_get_del(QemuOpts *opts, const char *name)
> +{
> + =C2=A0 =C2=A0const char *str =3D qemu_opt_get(opts, name);
> + =C2=A0 =C2=A0QemuOpt *opt =3D qemu_opt_find(opts, name);
> + =C2=A0 =C2=A0if (opt) {
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0qemu_opt_del(opt);

Is it any more efficient to reduce the number of times you crawl thro= ugh
the list? =C2=A0For example, instead of crawling the list with qemu_opt_get= ,
why not get teh string directly from the result of qemu_opt_find? =C2=A0Als= o,
you crawl again for qemu_opt_del. =C2=A0Maybe it's unavoidable, and cer= tainly
still O(n), so not the end of the world, but worth thinking about.

In previous Dong Xu's patch, it's commented that t= here is too much code
duplication in qemu_opt_get and qemu_opt_get_del,= could using qemu_opt_get
directly. Well, I'll improve that. Add "remove" flag to _qemu= _opt_get to handle
delete or not, and then wrapper _qemu_opt_get to gene= rate qemu_opt_get and
qemu_opt_get_del.

> +void qemu_opts_print_help(QemuOptsList *list)
> +{
> + =C2=A0 =C2=A0int i;
> + =C2=A0 =C2=A0printf("Supported options:\n");

Why printf? =C2=A0Might there be callers that want to print to somewh= ere
other than stdout?

Just followed print_= option_help. Existing print_option_help uses printf.
Could im= prove of course.

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


--089e0158a82aee49c004f2a897d5--