On 04/22/2018 04:47 AM, Simon Ruderich wrote:
> On Tue, Apr 17, 2018 at 04:18:43PM -0500, Eric Blake wrote:
>> Focusing on just the interface:
>>
>>> +++ b/qapi/misc.json
>>> @@ -1185,6 +1185,26 @@
>>>  { 'command': 'pmemsave',
>>>    'data': {'val': 'int', 'size': 'int', 'filename': 'str'} }
>>>
>>> +##
>>> +# @pmemload:
>>> +#
>>> +# Load a portion of guest physical memory from a file.
>>> +#
>>> +# @val: the physical address of the guest to start from
>>> +#
>>> +# @size: the size of memory region to load
>>
>> Should size be an optional parameter (default read to the end of the file)?
>>
>>> +#
>>> +# @offset: the offset in the file to start from
>>
>> Should offset be an optional parameter (default start reading from
>> offset 0 of the file)?
> 
>>From the QAPI point-of-view making both optional seems like a
> good idea, however then pmemsave should also mark its "size"
> parameter optional for consistency (this is backwards compatible
> as existing callers are not affected).

Correct.

> 
> On the monitor API side this is more problematic as the order of
> the pmemsave/pmemload parameters make optional parameters
> impossible.

Back-compat in the QMP interface matters.  The HMP interface, however,
exists to serve humans not machines, and we can break it at will to
something that makes more sense to humans.  So don't let HMP concerns
hold you back from a sane interface.

> 
> Personally I'd keep it as is because it's simple and consistent
> with the existing interface (and can be changed in the future
> without breaking compatibility).
> 
> In case you prefer those parameters to be optional I'll require
> some help to implement that: I don't understand how
> qapi-schema.json, hmp-commands.hx and hmp.h interact (I've to
> admit I just copied that from the original patch and didn't give
> it much thought as it worked fine) and would require some
> pointers on how to implement optional arguments for
> qapi-schema.json.

Optional parameters are listed as '*name':'type' in the .json file,
which tells the generator to create a 'has_name' bool parameter
alongside the 'name' parameter in the C code for the QMP interface.  The
HMP interface should then call into the QMP interface.

Recent HMP patches that I've authored may offer some inspiration: commit
08fb10a added a new command, and commit dba4932 added an optional
parameter to an existing command.

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