From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:42050) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1StLCg-0002OY-Cc for qemu-devel@nongnu.org; Mon, 23 Jul 2012 12:09:51 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1StLCa-00043Q-FW for qemu-devel@nongnu.org; Mon, 23 Jul 2012 12:09:50 -0400 Received: from mx1.redhat.com ([209.132.183.28]:21085) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1StLCa-00043I-81 for qemu-devel@nongnu.org; Mon, 23 Jul 2012 12:09:44 -0400 Date: Mon, 23 Jul 2012 13:10:18 -0300 From: Luiz Capitulino Message-ID: <20120723131018.73bfa314@doriath.home> In-Reply-To: <87k3xx1qoq.fsf@blackfin.pond.sub.org> References: <1342212261-19903-1-git-send-email-lcapitulino@redhat.com> <1342212261-19903-2-git-send-email-lcapitulino@redhat.com> <87k3xx1qoq.fsf@blackfin.pond.sub.org> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 1/8] qemu-option: qemu_opt_set_bool(): fix code duplication List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: jan.kiszka@siemens.com, aliguori@us.ibm.com, qemu-devel@nongnu.org, afaerber@suse.de On Sat, 21 Jul 2012 10:09:09 +0200 Markus Armbruster wrote: > Luiz Capitulino writes: > > > Call qemu_opt_set() instead of duplicating opt_set(). > > > > Signed-off-by: Luiz Capitulino > > --- > > qemu-option.c | 28 +--------------------------- > > 1 file changed, 1 insertion(+), 27 deletions(-) > > > > diff --git a/qemu-option.c b/qemu-option.c > > index bb3886c..2cb2835 100644 > > --- a/qemu-option.c > > +++ b/qemu-option.c > > @@ -677,33 +677,7 @@ void qemu_opt_set_err(QemuOpts *opts, const char > > *name, const char *value, > > > > int qemu_opt_set_bool(QemuOpts *opts, const char *name, bool val) > > { > > - QemuOpt *opt; > > - const QemuOptDesc *desc = opts->list->desc; > > - int i; > > - > > - for (i = 0; desc[i].name != NULL; i++) { > > - if (strcmp(desc[i].name, name) == 0) { > > - break; > > - } > > - } > > - if (desc[i].name == NULL) { > > - if (i == 0) { > > - /* empty list -> allow any */; > > - } else { > > - qerror_report(QERR_INVALID_PARAMETER, name); > > - return -1; > > - } > > - } > > - > > - opt = g_malloc0(sizeof(*opt)); > > - opt->name = g_strdup(name); > > - opt->opts = opts; > > - QTAILQ_INSERT_TAIL(&opts->head, opt, next); > > - if (desc[i].name != NULL) { > > - opt->desc = desc+i; > > - } > > - opt->value.boolean = !!val; > > - return 0; > > + return qemu_opt_set(opts, name, val ? "on" : "off"); > > } > > > > int qemu_opt_foreach(QemuOpts *opts, qemu_opt_loopfunc func, void *opaque, > > Does a bit more than just obvious code de-duplication. Two things in > particular: > > * Error reporting > > Old: qerror_report(); return -1 > > New: opt_set() and qemu_opt_set() cooperate, like this: > opt_set(): error_set(); return; > qemu_opt_set(): > if (error_is_set(&local_err)) { > qerror_report_err(local_err); > error_free(local_err); > return -1; > } > > I guess the net effect is the same. Not sure it's worth mentioning in > the commit message. The end result of calling qemu_opt_set_bool() is the same. The difference between qerror_report() and qerror_report_err() is that the former gets error information from the error call, while the latter gets error information from the Error object. But they do the same thing. > * New sets opt->str to either "on" or "off" depending on val, then lets > reconstructs the value with qemu_opt_parse(). Which can't fail then. > I figure the latter part has no further impact. But what about > setting opt->str? Is this a bug fix? I don't remember if opt->str is read after the QemuOpt object is built. If it's, then yes, this is a bug fix. Otherwise it just make the final QemuOpt object more 'conforming'.