Re: [Qemu-devel] [PATCH] hmp/(p)memsave: Allow >32bit file size

[Markus Armbruster <armbru@redhat.com>]

"Dr. David Alan Gilbert" <dgilbert@redhat.com> writes:

> * Thomas Huth (thuth@redhat.com) wrote:
>> On 24.07.2017 14:14, Dr. David Alan Gilbert (git) wrote:
>> > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>> > 
>> > memsave and pmemsave only take 32bit size arguments in HMP at the
>> > moment; let them take 64bit values.
>> > 
>> > Reported-by: Pierre Kim <admin@manateeshome.com>
>> > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
>> > ---
>> >  hmp-commands.hx | 4 ++--
>> >  hmp.c           | 4 ++--
>> >  2 files changed, 4 insertions(+), 4 deletions(-)
>> > 
>> > diff --git a/hmp-commands.hx b/hmp-commands.hx
>> > index 1941e19932..ddf77ae7ac 100644
>> > --- a/hmp-commands.hx
>> > +++ b/hmp-commands.hx
>> > @@ -829,7 +829,7 @@ ETEXI
>> >  
>> >      {
>> >          .name       = "memsave",
>> > -        .args_type  = "val:l,size:i,filename:s",
>> > +        .args_type  = "val:l,size:l,filename:s",
>> >          .params     = "addr size file",
>> >          .help       = "save to disk virtual memory dump starting at 'addr' of size 'size'",
>> >          .cmd        = hmp_memsave,
>> > @@ -843,7 +843,7 @@ ETEXI
>> >  
>> >      {
>> >          .name       = "pmemsave",
>> > -        .args_type  = "val:l,size:i,filename:s",
>> > +        .args_type  = "val:l,size:l,filename:s",
>> >          .params     = "addr size file",
>> >          .help       = "save to disk physical memory dump starting at 'addr' of size 'size'",
>> >          .cmd        = hmp_pmemsave,
>> > diff --git a/hmp.c b/hmp.c
>> > index bf1de747d5..dfbd615380 100644
>> > --- a/hmp.c
>> > +++ b/hmp.c
>> > @@ -1066,7 +1066,7 @@ void hmp_cpu(Monitor *mon, const QDict *qdict)
>> >  
>> >  void hmp_memsave(Monitor *mon, const QDict *qdict)
>> >  {
>> > -    uint32_t size = qdict_get_int(qdict, "size");
>> > +    uint64_t size = qdict_get_int(qdict, "size");
>> >      const char *filename = qdict_get_str(qdict, "filename");
>> >      uint64_t addr = qdict_get_int(qdict, "val");
>> >      Error *err = NULL;
>> > @@ -1083,7 +1083,7 @@ void hmp_memsave(Monitor *mon, const QDict *qdict)
>> >  
>> >  void hmp_pmemsave(Monitor *mon, const QDict *qdict)
>> >  {
>> > -    uint32_t size = qdict_get_int(qdict, "size");
>> > +    uint64_t size = qdict_get_int(qdict, "size");
>> >      const char *filename = qdict_get_str(qdict, "filename");
>> >      uint64_t addr = qdict_get_int(qdict, "val");
>> >      Error *err = NULL;
>> 
>> The "size" parameter of the qmp_memsave() and qmp_pmemsave() function is
>> a signed integer (int64_t) ... could we get into trouble here if the
>> integer is really big? E.g. should we make "size" here signed, too, and
>> then add a sanity check for "size >= 0" ?
>
> OK, yes, I'll fix that for the sizes;

I think we should fix QMP instead: use type 'size' instead of 'int' for
byte counts.  There might be more than just memsave and pmemsave.

>                                        qmp_pmemsave hangs with -ve sizes
> (which probably needs fixing another time).
>
> V.2 coming up.
>
> Dave
>
>>  Thomas
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK