From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:58502) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UZcin-0002xA-1h for qemu-devel@nongnu.org; Tue, 07 May 2013 03:54:04 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UZcik-0002fo-E2 for qemu-devel@nongnu.org; Tue, 07 May 2013 03:54:00 -0400 Received: from e36.co.us.ibm.com ([32.97.110.154]:39753) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UZcik-0002fU-3K for qemu-devel@nongnu.org; Tue, 07 May 2013 03:53:58 -0400 Received: from /spool/local by e36.co.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Tue, 7 May 2013 01:53:56 -0600 Received: from d03relay03.boulder.ibm.com (d03relay03.boulder.ibm.com [9.17.195.228]) by d03dlp02.boulder.ibm.com (Postfix) with ESMTP id 563653E4003F for ; Tue, 7 May 2013 01:53:38 -0600 (MDT) Received: from d03av05.boulder.ibm.com (d03av05.boulder.ibm.com [9.17.195.85]) by d03relay03.boulder.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id r477rrEA122532 for ; Tue, 7 May 2013 01:53:53 -0600 Received: from d03av05.boulder.ibm.com (loopback [127.0.0.1]) by d03av05.boulder.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id r477rq4k015373 for ; Tue, 7 May 2013 01:53:53 -0600 Message-ID: <5188B30D.8090408@linux.vnet.ibm.com> Date: Tue, 07 May 2013 15:53:49 +0800 From: Dong Xu Wang MIME-Version: 1.0 References: <1365575111-4476-1-git-send-email-wdongxu@linux.vnet.ibm.com> <1365575111-4476-5-git-send-email-wdongxu@linux.vnet.ibm.com> <87sj20l2p4.fsf@blackfin.pond.sub.org> <51888EC4.3040306@linux.vnet.ibm.com> <87fvxzdyu0.fsf@blackfin.pond.sub.org> In-Reply-To: <87fvxzdyu0.fsf@blackfin.pond.sub.org> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH V13 4/6] create some QemuOpts functons List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: kwolf@redhat.com, wdongxu@cn.ibm.com, qemu-devel@nongnu.org, stefanha@redhat.com On 2013/5/7 15:38, Markus Armbruster wrote: > Dong Xu Wang writes: > >> On 2013/5/6 20:20, Markus Armbruster wrote: >>> Dong Xu Wang writes: >>> >>>> These functions will be used in next commit. qemu_opt_get_(*)_del functions >>>> are used to make sure we have the same behaviors as before: after get an >>>> option value, options++. >>> >>> I don't understand the last sentence. >>> I will make the sentence clear next version. In block layer, I created 2 series of functions: 1) one is qemu_opt_get_del series, it is used to parse create options, after parsing one parameter, will delete it. 2) the other is qemu_opt_replace_set series, it will delete related opt and then add a new one. >>>> Signed-off-by: Dong Xu Wang >>>> --- >>>> include/qemu/option.h | 11 +++++- >>>> util/qemu-option.c | 103 ++++++++++++++++++++++++++++++++++++++++++++++---- >>>> 2 files changed, 105 insertions(+), 9 deletions(-) >>>> >>>> diff --git a/include/qemu/option.h b/include/qemu/option.h >>>> index c7a5c14..d63e447 100644 >>>> --- a/include/qemu/option.h >>>> +++ b/include/qemu/option.h >>>> @@ -108,6 +108,7 @@ struct QemuOptsList { >>>> }; >>>> >>>> const char *qemu_opt_get(QemuOpts *opts, const char *name); >>>> +const char *qemu_opt_get_del(QemuOpts *opts, const char *name); >>>> /** >>>> * qemu_opt_has_help_opt: >>>> * @opts: options to search for a help request >>>> @@ -121,13 +122,18 @@ const char *qemu_opt_get(QemuOpts *opts, const char *name); >>>> */ >>>> bool qemu_opt_has_help_opt(QemuOpts *opts); >>>> bool qemu_opt_get_bool(QemuOpts *opts, const char *name, bool defval); >>>> +bool qemu_opt_get_bool_del(QemuOpts *opts, const char *name, bool defval); >>>> uint64_t qemu_opt_get_number(QemuOpts *opts, const char *name, uint64_t defval); >>>> uint64_t qemu_opt_get_size(QemuOpts *opts, const char *name, uint64_t defval); >>>> +uint64_t qemu_opt_get_size_del(QemuOpts *opts, const char *name, >>>> + uint64_t defval); >>>> int qemu_opt_set(QemuOpts *opts, const char *name, const char *value); >>>> +int qemu_opt_replace_set(QemuOpts *opts, const char *name, const char *value); >>>> void qemu_opt_set_err(QemuOpts *opts, const char *name, const char *value, >>>> Error **errp); >>>> int qemu_opt_set_bool(QemuOpts *opts, const char *name, bool val); >>>> int qemu_opt_set_number(QemuOpts *opts, const char *name, int64_t val); >>>> +int qemu_opt_replace_set_number(QemuOpts *opts, const char *name, int64_t val); >>>> typedef int (*qemu_opt_loopfunc)(const char *name, const char *value, void *opaque); >>>> int qemu_opt_foreach(QemuOpts *opts, qemu_opt_loopfunc func, void *opaque, >>>> int abort_on_failure); >>>> @@ -144,7 +150,10 @@ const char *qemu_opts_id(QemuOpts *opts); >>>> void qemu_opts_del(QemuOpts *opts); >>>> void qemu_opts_validate(QemuOpts *opts, const QemuOptDesc *desc, Error **errp); >>>> int qemu_opts_do_parse(QemuOpts *opts, const char *params, const char *firstname); >>>> -QemuOpts *qemu_opts_parse(QemuOptsList *list, const char *params, int permit_abbrev); >>>> +int qemu_opts_do_parse_replace(QemuOpts *opts, const char *params, >>>> + const char *firstname); >>>> +QemuOpts *qemu_opts_parse(QemuOptsList *list, const char *params, >>>> + int permit_abbrev); >>>> void qemu_opts_set_defaults(QemuOptsList *list, const char *params, >>>> int permit_abbrev); >>>> QemuOpts *qemu_opts_from_qdict(QemuOptsList *list, const QDict *qdict, >>>> diff --git a/util/qemu-option.c b/util/qemu-option.c >>>> index 0488c27..5db6d76 100644 >>>> --- a/util/qemu-option.c >>>> +++ b/util/qemu-option.c >>>> @@ -33,6 +33,8 @@ >>>> #include "qapi/qmp/qerror.h" >>>> #include "qemu/option_int.h" >>>> >>>> +static void qemu_opt_del(QemuOpt *opt); >>>> + >>>> /* >>>> * Extracts the name of an option from the parameter string (p points at the >>>> * first byte of the option name) >>>> @@ -549,6 +551,16 @@ const char *qemu_opt_get(QemuOpts *opts, const char *name) >>> const char *qemu_opt_get(QemuOpts *opts, const char *name) >>> { >>> QemuOpt *opt = qemu_opt_find(opts, name); >>> const QemuOptDesc *desc; >>> desc = find_desc_by_name(opts->list->desc, name); >>> >>> return opt ? opt->str : >>>> (desc && desc->def_value_str ? desc->def_value_str : NULL); >>>> } >>>> >>>> +const char *qemu_opt_get_del(QemuOpts *opts, const char *name) >>>> +{ >>>> + QemuOpt *opt = qemu_opt_find(opts, name); >>>> + const char *str = opt ? g_strdup(opt->str) : NULL; >>>> + if (opt) { >>>> + qemu_opt_del(opt); >>>> + } >>>> + return str; >>>> +} >>>> + >>> >>> Unlike qemu_opt_del(), this one doesn't use def_value_str. Why? Isn't >>> that a trap for users of this function? >>> >>> Same question for the qemu_opt_get_FOO_del() that follow. > > I'm still interested in answers :) > Here I made a mistake, it should use def_value_str, will fix in next version. >>>> bool qemu_opt_has_help_opt(QemuOpts *opts) >>>> { >>>> QemuOpt *opt; >>>> @@ -577,6 +589,22 @@ bool qemu_opt_get_bool(QemuOpts *opts, const char *name, bool defval) >>>> return opt->value.boolean; >>>> } >>>> >>>> +bool qemu_opt_get_bool_del(QemuOpts *opts, const char *name, bool defval) >>>> +{ >>>> + QemuOpt *opt = qemu_opt_find(opts, name); >>>> + bool ret; >>>> + >>>> + if (opt == NULL) { >>>> + return defval; >>>> + } >>>> + ret = opt->value.boolean; >>>> + assert(opt->desc && opt->desc->type == QEMU_OPT_BOOL); >>>> + if (opt) { >>>> + qemu_opt_del(opt); >>>> + } >>>> + return ret; >>>> +} >>>> + >>>> uint64_t qemu_opt_get_number(QemuOpts *opts, const char *name, uint64_t defval) >>>> { >>>> QemuOpt *opt = qemu_opt_find(opts, name); >>>> @@ -609,6 +637,23 @@ uint64_t qemu_opt_get_size(QemuOpts *opts, const char *name, uint64_t defval) >>>> return opt->value.uint; >>>> } >>>> >>>> +uint64_t qemu_opt_get_size_del(QemuOpts *opts, const char *name, >>>> + uint64_t defval) >>>> +{ >>>> + QemuOpt *opt = qemu_opt_find(opts, name); >>>> + uint64_t ret; >>>> + >>>> + if (opt == NULL) { >>>> + return defval; >>>> + } >>>> + ret = opt->value.uint; >>>> + assert(opt->desc && opt->desc->type == QEMU_OPT_SIZE); >>>> + if (opt) { >>>> + qemu_opt_del(opt); >>>> + } >>>> + return ret; >>>> +} >>>> + >>>> static void qemu_opt_parse(QemuOpt *opt, Error **errp) >>>> { >>>> if (opt->desc == NULL) >>>> @@ -646,7 +691,7 @@ static bool opts_accepts_any(const QemuOpts *opts) >>>> } >>>> >>>> static void opt_set(QemuOpts *opts, const char *name, const char *value, >>>> - bool prepend, Error **errp) >>>> + bool prepend, bool replace, Error **errp) >>>> { >>>> QemuOpt *opt; >>>> const QemuOptDesc *desc; >>>> @@ -658,6 +703,11 @@ static void opt_set(QemuOpts *opts, const char *name, const char *value, >>>> return; >>>> } >>>> >>>> + opt = qemu_opt_find(opts, name); >>>> + if (replace && opt) { >>>> + qemu_opt_del(opt); >>>> + } >>>> + >>>> opt = g_malloc0(sizeof(*opt)); >>>> opt->name = g_strdup(name); >>>> opt->opts = opts; >>>> @@ -667,7 +717,6 @@ static void opt_set(QemuOpts *opts, const char *name, const char *value, >>>> QTAILQ_INSERT_TAIL(&opts->head, opt, next); >>>> } >>>> opt->desc = desc; >>>> - opt->str = g_strdup(value); >>>> opt->str = g_strdup(value ?: desc->def_value_str); >>>> qemu_opt_parse(opt, &local_err); >>>> if (error_is_set(&local_err)) { >>> >>> Here you plug the leak you created in PATCH 1/6. >>> >>>> @@ -680,7 +729,29 @@ int qemu_opt_set(QemuOpts *opts, const char *name, const char *value) >>>> { >>>> Error *local_err = NULL; >>>> >>>> - opt_set(opts, name, value, false, &local_err); >>>> + opt_set(opts, name, value, false, false, &local_err); >>>> + if (error_is_set(&local_err)) { >>>> + qerror_report_err(local_err); >>>> + error_free(local_err); >>>> + return -1; >>>> + } >>>> + >>>> + return 0; >>>> +} >>>> + >>>> +/* >>>> + * If name exists, the function will delete the opt first and then add a new >>>> + * one. >>>> + */ >>>> +int qemu_opt_replace_set(QemuOpts *opts, const char *name, const char *value) >>>> +{ >>>> + Error *local_err = NULL; >>>> + QemuOpt *opt = qemu_opt_find(opts, name); >>>> + >>>> + if (opt) { >>>> + qemu_opt_del(opt); >>>> + } >>> >>> Why? Won't opt_set() delete it already? >>> >> No, opt_set will not delete it, but add a new opt. In block layer, > > Then I'm confused... > >> while parsing options, if the parameter is parsed, it should be >> deleted and won't be used again, so I delete it manually. >> >>> Same question for the qemu_opt_replace_set_FOO() that follow. >>> >>>> + opt_set(opts, name, value, false, true, &local_err); > > ... because here you pass true for parameter replace, which I (naively?) > expect to delete the option. Can you unconfuse me? > Sorry, I used qemu_opt_del twice, will clean. >>>> if (error_is_set(&local_err)) { >>>> qerror_report_err(local_err); >>>> error_free(local_err); >>>> @@ -693,7 +764,7 @@ int qemu_opt_set(QemuOpts *opts, const char *name, const char *value) >>>> void qemu_opt_set_err(QemuOpts *opts, const char *name, const char *value, >>>> Error **errp) >>>> { >>>> - opt_set(opts, name, value, false, errp); >>>> + opt_set(opts, name, value, false, false, errp); >>>> } >>>> >>>> int qemu_opt_set_bool(QemuOpts *opts, const char *name, bool val) >>>> @@ -740,6 +811,16 @@ int qemu_opt_set_number(QemuOpts *opts, const char *name, int64_t val) >>>> return 0; >>>> } >>>> >>>> +int qemu_opt_replace_set_number(QemuOpts *opts, const char *name, int64_t val) >>>> +{ >>>> + QemuOpt *opt = qemu_opt_find(opts, name); >>>> + >>>> + if (opt) { >>>> + qemu_opt_del(opt); >>>> + } >>>> + return qemu_opt_set_number(opts, name, val); >>>> +} >>>> + >>>> int qemu_opt_foreach(QemuOpts *opts, qemu_opt_loopfunc func, void *opaque, >>>> int abort_on_failure) >>>> { >>>> @@ -917,7 +998,7 @@ int qemu_opts_print(QemuOpts *opts) >>>> } >>>> >>>> static int opts_do_parse(QemuOpts *opts, const char *params, >>>> - const char *firstname, bool prepend) >>>> + const char *firstname, bool prepend, bool replace) >>>> { >>>> char option[128], value[1024]; >>>> const char *p,*pe,*pc; >>>> @@ -953,7 +1034,7 @@ static int opts_do_parse(QemuOpts *opts, const char *params, >>>> } >>>> if (strcmp(option, "id") != 0) { >>>> /* store and parse */ >>>> - opt_set(opts, option, value, prepend, &local_err); >>>> + opt_set(opts, option, value, prepend, replace, &local_err); >>>> if (error_is_set(&local_err)) { >>>> qerror_report_err(local_err); >>>> error_free(local_err); >>>> @@ -969,7 +1050,13 @@ static int opts_do_parse(QemuOpts *opts, const char *params, >>>> >>>> int qemu_opts_do_parse(QemuOpts *opts, const char *params, const char *firstname) >>>> { >>>> - return opts_do_parse(opts, params, firstname, false); >>>> + return opts_do_parse(opts, params, firstname, false, false); >>>> +} >>>> + >>>> +int qemu_opts_do_parse_replace(QemuOpts *opts, const char *params, >>>> + const char *firstname) >>>> +{ >>>> + return opts_do_parse(opts, params, firstname, false, true); >>>> } >>>> >>>> static QemuOpts *opts_parse(QemuOptsList *list, const char *params, >>>> @@ -1008,7 +1095,7 @@ static QemuOpts *opts_parse(QemuOptsList *list, const char *params, >>>> return NULL; >>>> } >>>> >>>> - if (opts_do_parse(opts, params, firstname, defaults) != 0) { >>>> + if (opts_do_parse(opts, params, firstname, defaults, false) != 0) { >>>> qemu_opts_del(opts); >>>> return NULL; >>>> } >>> >>> > > >