From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:50842) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bAI6v-0003F1-4Q for qemu-devel@nongnu.org; Tue, 07 Jun 2016 10:36:06 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bAI6r-0000r7-W3 for qemu-devel@nongnu.org; Tue, 07 Jun 2016 10:36:05 -0400 Date: Tue, 7 Jun 2016 15:35:48 +0100 From: "Daniel P. Berrange" Message-ID: <20160607143548.GF20196@redhat.com> Reply-To: "Daniel P. Berrange" References: <1465294275-8733-1-git-send-email-berrange@redhat.com> <5756B854.9080600@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <5756B854.9080600@redhat.com> Subject: Re: [Qemu-devel] [PATCH v1 0/6] Report format specific info for LUKS block driver List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake Cc: qemu-devel@nongnu.org, qemu-block@nongnu.org, Kevin Wolf , Max Reitz , Markus Armbruster , Michael Roth On Tue, Jun 07, 2016 at 06:04:36AM -0600, Eric Blake wrote: > On 06/07/2016 04:11 AM, Daniel P. Berrange wrote: > > uuid: 6ddee74b-3a22-408c-8909-6789d4fa2594 > > slots: > > [0]: > > active: true > > iters: 572706 > > stripes: 4000 > > key-offset: 8 > > [1]: > > active: false > > iters: 0 > > stripes: 4000 > > key-offset: 264 > > [2]: > > active: false > > iters: 0 > > stripes: 4000 > > key-offset: 520 > > Is it worth making iters/stripes optional, and present only when active > is true? key-offset, on the other hand, should always be present. NB stripes is actually fixed at time of creation even if the slot is not active, but agre we could omit it, not least since it'll be the same for every slot anyway. > > The remaining 4 patches are improvements to QAPI and the core > > block layer to fix a problem whereby struct fields are output > > in (apparently) random ordering. This is because the QAPI type > > is converted into a QObject for pretty-printing, thus throwing > > away any struct field ordering information. > > > > To address this I created a new TextOutputVisitor which can > > directly pretty-print QAPI types. I then changed the code > > generator to create qapi_stringify_TYPENAME() methods for > > all QAPI types. Finally I changed the block layer over to > > use this stringify approach instead. > > Umm, how much does that duplicate my work on adding the JSON pretty > printer, which DOES print JSON in the same order as the underlying QAPI > struct? I still need to address Markus' latest reviews, but suspect we > may be able to combine efforts rather than duplicating. Looking at your code, it seems we've basically got a similar kind of structure to our visitors, but of course you're using the qstring routines to format in JSON, where as my visitor is doing a pretty-plain text formatting that's intended to be identical to what "qemu-img info" currently reports in plain text mode. > > I'm not sure if QAPI maintainers will find the idea of adding > > qapi_stringify_TYPENAME() methods desirable or not. It is of > > course valid to directly use the TextOutputVisitor from the > > block layer. I felt there might be some use in debugging to > > have a convenient qapi_stringify_TYPENAME() method around > > though - personally I often find it helpful to be able to > > easily dump an QAPI object or any QObject to a humand friendly > > format for debugging and the less code I need write to add > > this temporary debug output the better. > > I agree with that sentiment, but being able to dump directly to JSON > without going through an intermediate QObject may already be what you want. We'd have to write something that took the JSON string, parsed it and then pretty-printed in plain text. This actally sounds like more work than a visitor that directly outputs pretty plain text. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|