Re: [Qemu-devel] [PATCH V4 08/10] NUMA: add qmp command set-mpol to set memory policy for NUMA node

[Eric Blake]

On 07/04/2013 03:53 AM, Wanlong Gao wrote:
> The QMP command let it be able to set node's memory policy

s/let it be able/allows users/

> through the QMP protocol. The qmp-shell command is like:
>     set-mpol nodeid=0 mem-policy=membind mem-hostnode=0-1
> 
> Signed-off-by: Wanlong Gao <address@hidden>
> ---

Just an interface review:

> +++ b/qapi-schema.json
> @@ -3712,3 +3712,18 @@
>              '*cpuid-input-ecx': 'int',
>              'cpuid-register': 'X86CPURegister32',
>              'features': 'int' } }
> +
> +# @set-mpol:

I agree with other requests in this thread to make the name closer to
English words (set-memory-policy).  Also, I hate write-only interfaces;
what is the corresponding query-* command that lets me learn the policy
that is currently in effect?  I'm expecting that this series either
modifies an existing command or adds a new query command as the
counterpart to this set command.

> +#
> +# Set the host memory binding policy for guest NUMA node.
> +#
> +# @nodeid: The node ID of guest NUMA node to set memory policy to.
> +#
> +# @mem-policy: The memory policy string to set.
> +#
> +# @mem-hostnode: The host node or node range for memory policy.
> +#
> +# Since: 1.6.0
> +##
> +{ 'command': 'set-mpol', 'data': {'nodeid': 'int', '*mem-policy': 'str',
> +                                  '*mem-hostnode': 'str'} }

Make mem-policy an enum, not an open-coded string.  Also, make
mem-hostnode an array of nodes - a general rule of thumb is that if the
receiver (here, qemu) has to do further parsing of the data (such as
scraping out integer vs. dash to decide if it is one node or a range),
then the JSON was too high-level.  Using ['int'] instead of 'str' will
let the data be available already parsed into an integer list by the
visitor code, so you aren't having to write your own ad hoc parser on
the receiving end.

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

signature.asc
Description: OpenPGP digital signature