Re: [Qemu-devel] [PATCH v3 5/5] qmp: add pmemload command

[Markus Armbruster <armbru@redhat.com>]

Simon Ruderich <simon@ruderich.org> writes:

> On Fri, Aug 10, 2018 at 11:36:51AM +0100, Dr. David Alan Gilbert wrote:
>>> --- a/hmp-commands.hx
>>> +++ b/hmp-commands.hx

Subject claims "qmp: add", but the patch also adds to hmp.  Recommend to
split the patch into QMP and HMP part.

>>> @@ -822,6 +822,20 @@ STEXI
>>>  @item pmemsave @var{addr} @var{size} @var{file}
>>>  @findex pmemsave
>>>  save to disk physical memory dump starting at @var{addr} of size @var{size}.
>>> +ETEXI
>>> +
>>> +    {
>>> +        .name       = "pmemload",
>>> +        .args_type  = "val:l,size:i,offset:i,filename:s",
>>> +        .params     = "addr size offset file",
>>> +        .help       = "load from disk physical memory dump starting at 'addr' of size 'size' at file offset 'offset'",
>>> +        .cmd        = hmp_pmemload,
>>> +    },
>>> +
>>
>> I'm guessing that size and offset should be 'l' to allow large
>> sizes and offsets, and there's an 'F' type for filenames
>
> I've copied that from "pmemsave" and "memsave" which use 'i' for
> size. I'll add patches which will adapt them to use both 'l' and
> 'F' and adapt my pmemload patch as well.
>
> qapi/misc.json seems to always use 'int' for integer types. Is
> this value large enough on 64-bit architectures?

Yes.  QAPI's int translates to int64_t.

>> (see monitor.c which has a big comment table near the start).
>
> Thanks.
>
> Just curious, what is the difference between 's' and 'F'. Is that
> only for documentation purposes (and maybe tab completion) or is
> the usage different? I noticed existing code uses qdict_get_str()
> for both 's' and 'F'.

The main behavioral difference is completion.

>> Also, had you considered rearranging and making them optional,
>> for example if you do:
>>
>> val:l,filename:F,offset:i?,size:i?
>>
>> I think that would mean you can do the fairly obvious:
>>   pmemload addr "myfile"
>>
>> with the assumption that loads the whole file.
>
> I tried to keep it as similar to the existing functions
> "memsave"/"pmemsave", only adding "offset". Eric Blake already
> raised this issue in the thread, here's my response for
> reference:
>
> On Mon, Apr 23, 2018 at 02:46:57PM -0500, Eric Blake wrote:
>> 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.
>
> I see. However I don't like breaking existing interfaces unless I
> have to. In this case I think not having the optional parameters
> is fine and the consistency between the existing memsave/pmemsave
> functions is more important.
>
>> 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.
>
> Thank you for the explanation, this looks straight-forward.
>
> Do you have strong opinions regarding the optional parameters or
> would you accept the patch as is (minus possible implementation
> issues)? I like the symmetry to the existing functions (I noticed
> that size can only be optional for pmemload because saving the
> complete memory doesn't sound useful) and having to specify
> size/offset doesn't hurt the caller too much.

I recommend to start with the QMP interface.  Parameters are unordered
there.  memsave and pmemsave both take mandatory @val, @size, @filename.
memsave additionally takes optional @cpu-index.

Your pmemload has pmemsave's arguments plus and mandatory @offset.
Rationale for adding @offset?  You may have answered this question
already; pointer to that answer would be fine.

Once we got the QMP interface nailed down, we can move to the HMP
interface.

> Thanks for your review!
>
> Regards
> Simon
>
> PS: Diff between v3 and my current local version follows:
>
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index 5a43dae133..c39d745a22 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -818,7 +818,7 @@ ETEXI
>  
>      {
>          .name       = "memsave",
> -        .args_type  = "val:l,size:i,filename:s",
> +        .args_type  = "val:l,size:l,filename:F",
>          .params     = "addr size file",
>          .help       = "save to disk virtual memory dump starting at 'addr' of size 'size'",
>          .cmd        = hmp_memsave,
> @@ -832,7 +832,7 @@ ETEXI
>  
>      {
>          .name       = "pmemsave",
> -        .args_type  = "val:l,size:i,filename:s",
> +        .args_type  = "val:l,size:l,filename:F",
>          .params     = "addr size file",
>          .help       = "save to disk physical memory dump starting at 'addr' of size 'size'",
>          .cmd        = hmp_pmemsave,

These two should become a separate bug fix patch.  The bug being fixed
is completion.

> @@ -846,7 +846,7 @@ ETEXI
>  
>      {
>          .name       = "pmemload",
> -        .args_type  = "val:l,size:i,offset:i,filename:s",
> +        .args_type  = "val:l,size:l,offset:l,filename:F",
>          .params     = "addr size offset file",
>          .help       = "load from disk physical memory dump starting at 'addr' of size 'size' at file offset 'offset'",
>          .cmd        = hmp_pmemload,
> diff --git a/qapi/misc.json b/qapi/misc.json
> index 6c34b2ff8b..becc257a76 100644
> --- a/qapi/misc.json
> +++ b/qapi/misc.json
> @@ -1196,7 +1196,7 @@
>  #
>  # Returns: Nothing on success
>  #
> -# Since: 2.13
> +# Since: 3.1
>  ##
>  { 'command': 'pmemload',
>    'data': {'val': 'int', 'size': 'int', 'offset': 'int', 'filename': 'str'} }