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

[Markus Armbruster <armbru@redhat.com>]

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.

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) ?

> +
> +    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)