From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:39242) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aahId-00032A-70 for qemu-devel@nongnu.org; Tue, 01 Mar 2016 05:13:04 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aahIb-0007BQ-PD for qemu-devel@nongnu.org; Tue, 01 Mar 2016 05:13:03 -0500 Date: Tue, 1 Mar 2016 11:12:54 +0100 From: Kevin Wolf Message-ID: <20160301101254.GB4250@noname.str.redhat.com> References: <1456787973-19348-1-git-send-email-mreitz@redhat.com> <56D4D314.2070405@redhat.com> <56D4D62A.3060905@redhat.com> <20160301100035.GC26728@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20160301100035.GC26728@redhat.com> Subject: Re: [Qemu-devel] [PATCH v2 00/16] qapi: Allow blockdev-add for NBD List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Daniel P. Berrange" Cc: qemu-block@nongnu.org, Markus Armbruster , qemu-devel@nongnu.org, Luiz Capitulino , Paolo Bonzini , Max Reitz Am 01.03.2016 um 11:00 hat Daniel P. Berrange geschrieben: > On Tue, Mar 01, 2016 at 12:37:14AM +0100, Max Reitz wrote: > > On 01.03.2016 00:24, Eric Blake wrote: > > > On 02/29/2016 04:19 PM, Max Reitz wrote: > > >> Turns out NBD is not so simple to do if you do it right. Anyway, this > > >> series adds blockdev-add support for NBD clients. > > >> > > >> Patches 1 and 2 add one less and one more complicated QDict function, > > >> respectively, which I needed in later NBD patches: Patch 1 for handling > > >> legacy options (move "host" to "address.data.host" etc.) and patch 2 > > >> because I'd like to use the input visitor for transforming the NBD > > >> options into a SocketAddress. Unfortunately, the block layer uses > > >> flattened QDicts everywhere, so we'll have to unflatten (erect?) them > > >> before we can use that visitor. > > > > > > Dan had a patch proposal that called the operation "crumple"; I need to > > > review both proposals and see which one I like. > > > https://lists.gnu.org/archive/html/qemu-devel/2016-02/msg04618.html > > > > Well, here I go again, not looking at patches on the list... > > > > Looking at the design, I like his idea of having an escape sequence; > > also, his qdict_crumple() can return boths lists and dicts where my > > qdict_unflatten() only returns dicts (then again, this is what > > qdict_flatten() always works on). And his patch doesn't suffer from as > > much indentation as mine does. > > The escape sequence is critical to support for my use case, because > sadly some object properties have '.' in their name :-( > > > What I like more about my patch, however, is that I'm reusing > > qdict_array_split() and qdict_array_entries(). That is mostly because my > > function modifies the given QDict, where Dan's does not. > > The reason for that is that the use context in which I need to call > qdict_crumple() has a const QDict, so modifying the original QDict > was not an option. You can always clone and modify if modifying an existing QDict turns out to be nicer to implement. > Second, for error handling, if there is a problem we can't resolve > half way through the unflattening process, then if you're modifying > the original QDict you end up with a QDict that is a hybrid between > the flat & unflat forms. I think it is pretty bad practice for API > design / behaviour to leave inputs in such a state on error. ie if > the code isn't capable of rolling back to the original state it > should not be modifying the input arg. I think we generally abort the whole action in such error cases. Then it doesn't really matter in what state the to be freed QDict is. Kevin