Re: [PATCH v3] qmp: Use unsigned integers for address parameters

[Josh Junon]

On Fri, Aug 02, 2024 at 04:18:40PM +0200, Markus Armbruster wrote:
> Let's go back to v1's title
> 
>     qmp: Fix higher half vaddrs for [p]memsave
> 
> Josh Junon <junon@oro.sh> writes:
> 
> > Fixes higher-half address parsing for QMP commands
> > `[p]memsave`.
> >
> > Signed-off-by: Josh Junon <junon@oro.sh>
> > ---
> >  qapi/machine.json | 11 +++++++++--
> >  system/cpus.c     | 10 +++++-----
> >  2 files changed, 14 insertions(+), 7 deletions(-)
> >
> > diff --git a/qapi/machine.json b/qapi/machine.json
> > index fcfd249e2d..fb618dc99f 100644
> > --- a/qapi/machine.json
> > +++ b/qapi/machine.json
> > @@ -852,7 +852,11 @@
> >  #     <- { "return": {} }
> >  ##
> >  { 'command': 'memsave',
> > -  'data': {'val': 'int', 'size': 'int', 'filename': 'str', '*cpu-index': 
> > 'int'} }
> > +  'data': {
> > +     'val': 'uint64',
> > +     'size': 'size',
> > +     'filename': 'str',
> > +     '*cpu-index': 'int' } }
> >  
> >  ##
> >  # @pmemsave:
> > @@ -878,7 +882,10 @@
> >  #     <- { "return": {} }
> >  ##
> >  { 'command': 'pmemsave',
> > -  'data': {'val': 'int', 'size': 'int', 'filename': 'str'} }
> > +  'data': {
> > +    'val': 'uint64',
> > +    'size': 'size',
> > +    'filename': 'str' } }
> >  
> >  ##
> >  # @Memdev:
> > diff --git a/system/cpus.c b/system/cpus.c
> > index 5e3a988a0a..f33e4fbff0 100644
> > --- a/system/cpus.c
> > +++ b/system/cpus.c
> > @@ -792,14 +792,14 @@ int vm_stop_force_state(RunState state)
> >      }
> >  }
> >  
> > -void qmp_memsave(int64_t addr, int64_t size, const char *filename,
> > +void qmp_memsave(uint64_t addr, uint64_t size, const char *filename,
> >                   bool has_cpu, int64_t cpu_index, Error **errp)
> >  {
> >      FILE *f;
> > -    uint32_t l;
> > +    uint64_t l;
> >      CPUState *cpu;
> >      uint8_t buf[1024];
> > -    int64_t orig_addr = addr, orig_size = size;
> > +    uint64_t orig_addr = addr, orig_size = size;
> >  
> >      if (!has_cpu) {
> >          cpu_index = 0;
>        }
>    [...]
>            if (cpu_memory_rw_debug(cpu, addr, buf, l, 0) != 0) {
>                error_setg(errp, "Invalid addr 0x%016" PRIx64 "/size %" PRId64
>                                 " specified", orig_addr, orig_size);
> 
> @size is now unsigned and should be formatted with PRIu64.

Ah yes, missed that one last round. Apologies!

> 
>                goto exit;
>            }
> > @@ -840,11 +840,11 @@ exit:
> >      fclose(f);
> >  }
> >  
> > -void qmp_pmemsave(int64_t addr, int64_t size, const char *filename,
> > +void qmp_pmemsave(uint64_t addr, uint64_t size, const char *filename,
> >                    Error **errp)
> >  {
> >      FILE *f;
> > -    uint32_t l;
> > +    uint64_t l;
> >      uint8_t buf[1024];
> >  
> >      f = fopen(filename, "wb");
> 
> I can address my nitpicks in my tree, no respin necessary.
> 
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
> 

Sounds good to me, thank you for the reviews!

- Josh