From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:40947) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WPIUF-0005c5-My for qemu-devel@nongnu.org; Sun, 16 Mar 2014 17:20:57 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WPIU8-0006Nv-U4 for qemu-devel@nongnu.org; Sun, 16 Mar 2014 17:20:51 -0400 Received: from mail-ob0-f171.google.com ([209.85.214.171]:47114) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WPIU8-0006Nn-PR for qemu-devel@nongnu.org; Sun, 16 Mar 2014 17:20:44 -0400 Received: by mail-ob0-f171.google.com with SMTP id wn1so4775187obc.30 for ; Sun, 16 Mar 2014 14:20:44 -0700 (PDT) Date: Sun, 16 Mar 2014 21:19:37 +0000 From: Leandro Dorileo Message-ID: <20140316211937.GA23454@dorilex> References: <1394436721-21812-1-git-send-email-cyliu@suse.com> <1394436721-21812-4-git-send-email-cyliu@suse.com> <531E20AB.3090409@redhat.com> <531E2CC0.8080406@redhat.com> <20140311210004.GA23555@dorilex> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20140311210004.GA23555@dorilex> Subject: Re: [Qemu-devel] [PATCH v22 03/25] improve some functions in qemu-option.c List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Chunyan Liu Cc: Kevin Wolf , Dong Xu Wang , qemu-devel@nongnu.org, stefanha@redhat.com Hi Chunyan, On Tue, Mar 11, 2014 at 09:00:04PM +0000, Leandro Dorileo wrote: > Hi, > > On Tue, Mar 11, 2014 at 03:26:51PM +0800, Chunyan Liu wrote: > > 2014-03-11 5:21 GMT+08:00 Eric Blake : > > > > > On 03/10/2014 02:29 PM, Eric Blake wrote: > > > > > > >> + opt = qemu_opt_find(opts, name); > > > >> + if (opt) { > > > >> + g_free((char *)opt->str); > > > > > > > > ...which means the cast is pointless here. > > > > > > > > Hmm. This means that you are giving opt_set() the behavior of 'last > > > > version wins', by silently overwriting earlier versions. If I'm > > > > understanding the existing code correctly, the previous behavior was > > > > that calling opt_set twice in a row on the same name would inject BOTH > > > > names into 'opts', but then subsequent lookups on opts would find the > > > > FIRST hit. Doesn't that mean this is a semantic change: > > > > > > > > qemu -opt key=value1,key=value2 > > > > > > > > would previously set key to value1, but now sets key to value2. > > > > > > I've played with this a bit more, and now am more confused. QemuOpts is > > > a LOT to comprehend. > > > > > > Pre-patch, 'qemu-system-x86_64 -nodefaults -machine > > > type=none,type-noone' displayed a help message about unknown machine > > > type "noone", while swapping type=noone,type=none proceeded with the > > > 'none' type. So the last version silently won, which was not the > > > behavior I had predicted. > > > > > > Post-patch, I get a compilation error (so how did you test your patch?): > > > > > > > Mostly tested ./qemu-img commands where QEMUOptionParameter is used. > > I really didn't think of test QemuOpts fully, and about the test suite, I > > have no full > > knowledge about how many things need to be tested, how many things need to > > be > > covered? > > The testsuite should test the QemuOpts implementation, not the current users. I have just posted a patch introducing a basic QemuOpt testsuite, we can use it as a start. Regards... -- Leandro Dorileo