From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:55561) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UZaIx-0005BP-Gn for qemu-devel@nongnu.org; Tue, 07 May 2013 01:19:13 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UZaIv-0007yB-86 for qemu-devel@nongnu.org; Tue, 07 May 2013 01:19:11 -0400 Received: from e32.co.us.ibm.com ([32.97.110.150]:41693) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UZaIu-0007y7-VY for qemu-devel@nongnu.org; Tue, 07 May 2013 01:19:09 -0400 Received: from /spool/local by e32.co.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Mon, 6 May 2013 23:19:06 -0600 Received: from d03relay01.boulder.ibm.com (d03relay01.boulder.ibm.com [9.17.195.226]) by d03dlp02.boulder.ibm.com (Postfix) with ESMTP id 7E6813E4003E for ; Mon, 6 May 2013 23:18:48 -0600 (MDT) Received: from d03av06.boulder.ibm.com (d03av06.boulder.ibm.com [9.17.195.245]) by d03relay01.boulder.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id r475J2Fk131488 for ; Mon, 6 May 2013 23:19:02 -0600 Received: from d03av06.boulder.ibm.com (loopback [127.0.0.1]) by d03av06.boulder.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id r475LxtF012333 for ; Mon, 6 May 2013 23:21:59 -0600 Message-ID: <51888EC4.3040306@linux.vnet.ibm.com> Date: Tue, 07 May 2013 13:19:00 +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> In-Reply-To: <87sj20l2p4.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/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. > >> 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. > >> 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, 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); >> 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; >> } > >