Re: [Qemu-devel] [PATCH v20 04/26] add some QemuOpts functions for replace work

[Chunyan Liu]

2014-02-13 7:31 GMT+08:00 Eric Blake <address@hidden>:

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 <address@hidden>
> Signed-off-by: Chunyan Liu <address@hidden>
> ---
>  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