Re: [Qemu-devel] [PATCH 07/27] add memdev backend infrastructure

[Eric Blake]

On 11/20/2013 07:38 PM, Igor Mammedov wrote:
> Provides framework for splitting host RAM allocation/
> policies into a separate backend that could be used
> by devices. It would allow to separate host specific
> options from device model like it's done for netdev & co.

Just an interface review:

> +++ b/hmp-commands.hx
> @@ -1719,3 +1719,16 @@ ETEXI
>  STEXI
>  @end table
>  ETEXI
> +
> +    {
> +        .name       = "memdev-add",

Typically, we've been naming HMP commands with underscore: memdev_add

> +++ b/qapi-schema.json
> @@ -4212,3 +4212,21 @@
>  # Since: 1.7
>  ##
>  { 'command': 'blockdev-add', 'data': { 'options': 'BlockdevOptions' } }
> +
> +##
> +# @memdev_add:

Whereas QMP commands have a dash: memdev-add

> +#
> +# Add a host memory backend.
> +#
> +# @id: the name of the new memory backend
> +# @size: amount of memory backend should allocate

Maybe add "in bytes" somewhere in the phrase to make it clear

> +# @type: backend type. [default: compat-ram-host-memory]

That's the default, but what other types are valid?  I'd much rather
have an enum of valid types than an open-coded string.

> +#
> +# Since: 1.8
> +#
> +# Returns: Nothing on success
> +##
> +{ 'command': 'memdev-add',

Ah, you did name the QMP command with dash, so it's the docs above that
are wrong.

> +  'data': {'id': 'str', 'size': 'size', '*type': 'str'},
> +  'gen': 'no'
> +}

> +Arguments:
> +
> +- "id": the device's ID, must be unique (json-string)
> +- "size": amount of memory backend should allocate (json-int)
> +- "type": backend type (json-string, optional), default: 
> compat-ram-host-memory
> +
> +Example:
>  
> +-> { "execute": "memdev-add",
> +     "arguments": { "id": "memdev1",
> +                    "size": "1G",

I don't think this is valid QMP; you want to be passing sizes in bytes
as an integer, not as a string that requires further parsing.

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

signature.asc
Description: OpenPGP digital signature