Re: [Qemu-devel] [PATCH v3 10/12] Dump: add qmp command "query-dump"

[Peter Xu <peterx@redhat.com>]

On Mon, Nov 30, 2015 at 03:17:01PM -0700, Eric Blake wrote:
> On 11/30/2015 04:32 AM, Peter Xu wrote:
> > +DumpQueryResult *qmp_query_dump(Error **errp)
> > +{
> > +    DumpQueryResult *result = g_malloc0(sizeof(*result));
> 
> Might be nicer as g_new0(DumpQueryResult, 1). Markus has been switching
> to g_new0 where a type name was already mentioned, although here you
> used *result rather than a typename.

Sure. Thanks.

> 
> 
> > +++ b/qapi-schema.json
> > @@ -2157,6 +2157,35 @@
> >    'data': [ 'none', 'active', 'completed', 'failed' ] }
> >  
> >  ##
> > +# @DumpQueryResult
> > +#
> > +# The result format for 'query-dump'.
> > +#
> > +# @status: enum of @DumpStatus, which shows current dump status
> > +#
> > +# @written_bytes: bytes written in latest dump (uncompressed)
> > +#
> > +# @total_bytes: total bytes to be write in latest dump (uncompressed)
> 
> s/be write/written/
> 
> > +#
> > +# Since 2.6
> > +##
> > +{ 'struct': 'DumpQueryResult',
> > +  'data': { 'status': 'DumpStatus',
> > +            'written_bytes': 'int',
> > +            'total_bytes': 'int' } }
> 
> Prefer '-' over '_' in new QMP (as in 'total-bytes' rather than
> 'total_bytes').  Furthermore, QMP already defaults to bytes, so it would
> be sufficient to name these merely 'written'/'total' or even
> 'complete'/'total'.

I will directly take "completed" and "total" then.

> 
> > +++ b/qmp-commands.hx
> > @@ -881,7 +881,7 @@ EQMP
> >      {
> >          .name       = "query-dump-guest-memory-capability",
> >          .args_type  = "",
> > -    .mhandler.cmd_new = qmp_marshal_query_dump_guest_memory_capability,
> > +        .mhandler.cmd_new = qmp_marshal_query_dump_guest_memory_capability,
> >      },
> 
> Unrelated hunk.

Is it ok to have this line here? I just saw it and fixed it. Please
let me know if I should move it out of this patch.

Thanks.
Peter

> 
> -- 
> Eric Blake   eblake redhat com    +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
>