Re: [Qemu-devel] [PATCH 3/3] hmp: dump-guest-memory: hardcode protocol argument to file:

[Luiz Capitulino <lcapitulino@redhat.com>]

On Tue, 25 Sep 2012 10:48:05 +0200
Markus Armbruster <armbru@redhat.com> wrote:

> Luiz Capitulino <lcapitulino@redhat.com> writes:
> 
> > Today, it's necessary to specify the protocol you want to use
> > when dumping the guest memory, for example:
> >
> >  (qemu) dump-guest-memory file:/tmp/guest-memory
> >
> > This has a few issues:
> >
> >  1. It's cumbersome to type
> >  2. We loose file path autocompletion
> >  3. Being able to specify fd:X in HMP makes little sense for humans
> >
> > Because of these reasons, hardcode the 'protocol' argument to
> > 'file:' in HMP.
> 
> No objection.
> 
> The QMP wart remains: protocol is a string that needs to be parsed.  I
> feel that should be avoided in QMP.  We should have expressed "either
> filename or file descriptor name" in the schema.  Let's avoid such
> mistakes in the future.

I think that the right solution would be to have an URI.

> One remark inline.
> 
> > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> > ---
> >  hmp-commands.hx |  8 +++-----
> >  hmp.c           | 11 ++++++++---
> >  2 files changed, 11 insertions(+), 8 deletions(-)
> >
> > diff --git a/hmp-commands.hx b/hmp-commands.hx
> > index ed67e99..0302458 100644
> > --- a/hmp-commands.hx
> > +++ b/hmp-commands.hx
> > @@ -914,12 +914,11 @@ ETEXI
> >  #if defined(CONFIG_HAVE_CORE_DUMP)
> >      {
> >          .name       = "dump-guest-memory",
> > -        .args_type  = "paging:-p,protocol:s,begin:i?,length:i?",
> > -        .params     = "[-p] protocol [begin] [length]",
> > +        .args_type  = "paging:-p,filename:F,begin:i?,length:i?",
> > +        .params     = "[-p] filename [begin] [length]",
> >          .help       = "dump guest memory to file"
> >                        "\n\t\t\t begin(optional): the starting physical address"
> >                        "\n\t\t\t length(optional): the memory size, in bytes",
> > -        .user_print = monitor_user_noop,
> >          .mhandler.cmd = hmp_dump_guest_memory,
> >      },
> >  
> > @@ -929,8 +928,7 @@ STEXI
> >  @findex dump-guest-memory
> >  Dump guest memory to @var{protocol}. The file can be processed with crash or
> >  gdb.
> > -  protocol: destination file(started with "file:") or destination file
> > -            descriptor (started with "fd:")
> > +  filename: dump file name
> >      paging: do paging to get guest's memory mapping
> >       begin: the starting physical address. It's optional, and should be
> >              specified with length together.
> > diff --git a/hmp.c b/hmp.c
> > index ba6fbd3..513b40b 100644
> > --- a/hmp.c
> > +++ b/hmp.c
> > @@ -1042,11 +1042,12 @@ void hmp_dump_guest_memory(Monitor *mon, const QDict *qdict)
> >  {
> >      Error *errp = NULL;
> >      int paging = qdict_get_try_bool(qdict, "paging", 0);
> > -    const char *file = qdict_get_str(qdict, "protocol");
> > +    const char *file = qdict_get_str(qdict, "filename");
> >      bool has_begin = qdict_haskey(qdict, "begin");
> >      bool has_length = qdict_haskey(qdict, "length");
> >      int64_t begin = 0;
> >      int64_t length = 0;
> > +    QString *prot;
> >  
> >      if (has_begin) {
> >          begin = qdict_get_int(qdict, "begin");
> > @@ -1055,9 +1056,13 @@ void hmp_dump_guest_memory(Monitor *mon, const QDict *qdict)
> >          length = qdict_get_int(qdict, "length");
> >      }
> >  
> > -    qmp_dump_guest_memory(paging, file, has_begin, begin, has_length, length,
> > -                          &errp);
> > +    prot = qstring_from_str("file:");
> > +    qstring_append(prot, file);
> 
> What about prot = g_strconcat("file:", file, NULL) ... g_free(prot) ?

Good suggestion.

> 
> > +
> > +    qmp_dump_guest_memory(paging, qstring_get_str(prot), has_begin, begin,
> > +                          has_length, length, &errp);
> >      hmp_handle_error(mon, &errp);
> > +    QDECREF(prot);
> >  }
> >  
> >  void hmp_netdev_add(Monitor *mon, const QDict *qdict)
>