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

[Igor Mammedov]

On Wed, 27 Nov 2013 08:25:22 -0700
Eric Blake <address@hidden> wrote:

> 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
there is several DASH command in HMP and I thought we stopped to
do UNDERSCORE and that DASH is preffered way now.

> 
> > +++ 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
I'll fix it.

> 
> > +#
> > +# 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
Sure 
> 
> > +# @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.
there is only one so far, I plan to add additional hugepage backend later.

> 
> > +#
> > +# 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.
I'll amend it.

Thanks!