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

[Dr. David Alan Gilbert]

* Simon Ruderich (address@hidden) wrote:
> On Fri, Aug 10, 2018 at 11:36:51AM +0100, Dr. David Alan Gilbert wrote:
> >> --- a/hmp-commands.hx
> >> +++ b/hmp-commands.hx
> >> @@ -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?
> 
> > (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'.
> 
> > 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.

No strong feeling either way; that was just a suggestion.

Dave

> 
> 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,
> @@ -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'} }
> 
> -- 
> + privacy is necessary
> + using gnupg http://gnupg.org
> + public key id: 0x92FEFDB7E44C32F9


--
Dr. David Alan Gilbert / address@hidden / Manchester, UK