Re: [Qemu-block] [PATCH v1 0/6] Report format specific info for LUKS block driver

[Daniel P. Berrange]

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 :|