Re: [Qemu-devel] [PATCH] HMP: support specifying dump format for dump-guest-memory

[Markus Armbruster <armbru@redhat.com>]

Qiao Nuohan <qiaonuohan@cn.fujitsu.com> writes:

> Dumping guest memory is available to specify the dump format now. This patch
> adds options '-z|-l|-s' to HMP command dump-guest-memory to specify dumping in
> kdump-compression format, with zlib/lzo/snappy compression. And without these
> options ELF format will be used.
>
> The discussion about this feature is here:
>
> http://lists.nongnu.org/archive/html/qemu-devel/2014-03/msg04235.html
>
> Signed-off-by: Qiao Nuohan <qiaonuohan@cn.fujitsu.com>
> Suggested-by: Christian Borntraeger <borntraeger@de.ibm.com>
> ---
>  hmp-commands.hx | 11 +++++++----
>  hmp.c           | 25 ++++++++++++++++++++++++-
>  2 files changed, 31 insertions(+), 5 deletions(-)
>
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index f3fc514..4b9989f 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -998,8 +998,8 @@ ETEXI
>  
>      {
>          .name       = "dump-guest-memory",
> -        .args_type  = "paging:-p,filename:F,begin:i?,length:i?",
> -        .params     = "[-p] filename [begin] [length]",
> +        .args_type  = "paging:-p,zlib:-z,lzo:-l,snappy:-s,filename:F,begin:i?,length:i?",
> +        .params     = "[-p] [-z|-l|-s] filename [begin] [length]",

Since you're touching it anyway, please fix it to [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",

.help doesn't cover the flags.  Please fix that.

Both "(optional)" are redundant, because help also prints .params.

> @@ -1008,12 +1008,15 @@ ETEXI
>  
>  
>  STEXI
> -@item dump-guest-memory [-p] @var{protocol} @var{begin} @var{length}
> +@item dump-guest-memory [-p] [-z|-l|-s] @var{protocol} @var{begin} @var{length}
>  @findex dump-guest-memory
>  Dump guest memory to @var{protocol}. The file can be processed with crash or
> -gdb.
> +gdb. Without -z|-l|-s, the dump format is ELF.
>    filename: dump file name

This was called "protocol" above.  Not your fault, but please fix it to
"filename" anyway.

>      paging: do paging to get guest's memory mapping

This should be "-p:", not "paging:".  Again, not your fault, but please
fix it.

> +      zlib: dump in kdump-compressed format, with zlib compression
> +       lzo: dump in kdump-compressed format, with lzo compression
> +    snappy: dump in kdump-compressed format, with snappy compression

These should be "-z:", "-l:" and "-s:".

>       begin: the starting physical address. It's optional, and should be
>              specified with length together.
>      length: the memory size, in bytes. It's optional, and should be specified
               with begin together.

Please document that -p and begin/length both conflict with -z, -l, -s.

Here's a rather formal way to do that:

    @item dump-guest-memory [[-p] @var{filename} @var{begin} @var{length} | [-z|-l|-s] @var{filename}]

Awkward.  This might come out better:

    @item dump-guest-memory [-p] @var{filename} @var{begin} @var{length}
    @item dump-guest-memory [-z|-l|-s] @var{filename}

Alternatively describe the restrictions in the text rather than the 

Suggest to try and check the resulting .info.

In case you decide to touch the lines on begin and length for some
reason: "with length together" should be "together with length".

I'd probably fix the preexisting issues in a separate patch first.

> diff --git a/hmp.c b/hmp.c
> index 2f279c4..37c3961 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -1308,16 +1308,39 @@ void hmp_dump_guest_memory(Monitor *mon, const QDict *qdict)
>  {
>      Error *errp = NULL;
>      int paging = qdict_get_try_bool(qdict, "paging", 0);
> +    int zlib = qdict_get_try_bool(qdict, "zlib", 0);
> +    int lzo = qdict_get_try_bool(qdict, "lzo", 0);
> +    int snappy = qdict_get_try_bool(qdict, "snappy", 0);
>      const char *file = qdict_get_str(qdict, "filename");
>      bool has_begin = qdict_haskey(qdict, "begin");
>      bool has_length = qdict_haskey(qdict, "length");
> -    /* kdump-compressed format is not supported for HMP */
>      bool has_format = false;

Suggest to drop has_format, and pass true to qmp_dump_guest_memory()
instead.

>      int64_t begin = 0;
>      int64_t length = 0;
>      enum DumpGuestMemoryFormat dump_format = DUMP_GUEST_MEMORY_FORMAT_ELF;
>      char *prot;
>  
> +    if ((zlib + lzo + snappy) > 1) {

Superfluous parentheses around sum, please drop them.

> +        error_setg(&errp, "only one of '-z|-l|-s' can be set");
> +        hmp_handle_error(mon, &errp);
> +        return;
> +    }
> +
> +    if (zlib) {
> +        has_format = true;
> +        dump_format = DUMP_GUEST_MEMORY_FORMAT_KDUMP_ZLIB;
> +    }
> +
> +    if (lzo) {
> +        has_format = true;
> +        dump_format = DUMP_GUEST_MEMORY_FORMAT_KDUMP_LZO;
> +    }
> +
> +    if (snappy) {
> +        has_format = true;
> +        dump_format = DUMP_GUEST_MEMORY_FORMAT_KDUMP_SNAPPY;
> +    }
> +
>      if (has_begin) {
>          begin = qdict_get_int(qdict, "begin");
>      }