Re: [Qemu-devel] [PATCH v6 2/3] qmp: introduce query-memory-size-summary command

[Eric Blake]

On 08/16/2017 06:06 AM, Vadim Galitsyn wrote:
> Command above provides the following memory information in bytes:

My general preference for reading a commit message is to treat the
subject line as a one-line summary (the what), and then the commit
message body as something that can be read independently, starting with
an implicit "Apply this patch to...".  Your sentence reads awkwardly in
that light ("Apply this patch to command above provides...").  Better
might be:

Add a new query-memory-size-summary command which provides...

Yes, it repeats some of the subject line, but remember, not all mail
clients display the subject line immediately adjacent and in the same
font as the body, so making the reader refer back to the subject to get
context can be a distraction.

> 
>   * base-memory - size of "base" memory specified with command line option -m.
> 
>   * plugged-memory - amount of memory that was hot-plugged.
>     If target does not have CONFIG_MEM_HOTPLUG enabled, no
>     value is reported.
> 

> ---
>  hw/mem/pc-dimm.c                |  5 +++++
>  include/hw/mem/pc-dimm.h        |  1 +
>  qapi-schema.json                | 25 +++++++++++++++++++++++++
>  qmp.c                           | 13 +++++++++++++
>  stubs/Makefile.objs             |  2 +-
>  stubs/qmp_pc_dimm.c             | 13 +++++++++++++
>  stubs/qmp_pc_dimm_device_list.c |  8 --------
>  7 files changed, 58 insertions(+), 9 deletions(-)

You may want to look at using scripts/git.orderfile, to rearrange your
patch so that interface changes (.json, .h) occur before implementation
(.c).  For now, I'm just focusing on the interface:

> +++ b/qapi-schema.json
> @@ -4408,6 +4408,31 @@
>              '*unavailable-features': [ 'str' ], 'typename': 'str' } }
>  
>  ##
> +# @MemoryInfo:
> +#
> +# Actual memory information in bytes.
> +#
> +# @base-memory: size of "base" memory specified with command line
> +#               option -m.
> +#
> +# @plugged-memory: size memory that can be hot-unplugged.

Please also document when this field will be omitted.

> +#
> +# Since: 2.10.0

You missed 2.10.  This must be 2.11.

> +##
> +{ 'struct': 'MemoryInfo',
> +  'data'  : { 'base-memory': 'size', '*plugged-memory': 'size' } }
> +
> +##
> +# @query-memory-size-summary:
> +#
> +# Return the amount of initially allocated and hot-plugged (if
> +# enabled) memory in bytes.
> +#
> +# Since: 2.10.0

2.11

Also, consider including an example usage (there are efforts underway to
add automatic testing based on the examples, and a query-* command
should be easy to test in that manner).

> +##
> +{ 'command': 'query-memory-size-summary', 'returns': 'MemoryInfo' }
> +
> +##
>  # @query-cpu-definitions:
>  #


-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

signature.asc
Description: OpenPGP digital signature