Re: [Qemu-devel] [PATCH 12/13 v7] dump: make kdump-compressed format available for 'dump-guest-memory'

[Laszlo Ersek <lersek@redhat.com>]

Thanks for addressing my earlier comments. Some new ones below:

On 01/17/14 08:46, qiaonuohan wrote:

> +    /* check whether lzo/snappy is supported */
> +#ifndef CONFIG_LZO
> +    if (has_format && format == DUMP_GUEST_MEMORY_FORMAT_KDUMP_LZO) {
> +        error_setg(errp, "kdump-lzo is not available now");
> +    }
> +#endif
> +
> +#ifndef CONFIG_SNAPPY
> +    if (has_format && format == DUMP_GUEST_MEMORY_FORMAT_KDUMP_SNAPPY) {
> +        error_setg(errp, "kdump-snappy is not available now");
> +    }
> +#endif

Ekaterina caught in testing that these two blocks must be complemented
with a return statement each -- when you detect these errors,
qmp_dump_guest_memory() must not proceed. Apologies for not noticing
them in v6.

> +##
>  # @dump-guest-memory
>  #
>  # Dump guest's memory to vmcore. It is a synchronous operation that can take
> @@ -2712,13 +2730,18 @@
>  #          want to dump all guest's memory, please specify the start @begin
>  #          and @length
>  #
> +# @format: #optional if specified, the format of guest memory dump. But non-elf
> +#          format is conflict with paging and filter, ie. @paging, @begin and
> +#          @length is not allowed to be specified with @format at the same time

Please make it more precise with "non-elf", as in:

  ... are not allowed to be specified with *non-elf* @format at the same
  time

Not really relevant but since you'll respin anyway... :)

> +#          (since 2.0)
> +#
>  # Returns: nothing on success
>  #
>  # Since: 1.2
>  ##
>  { 'command': 'dump-guest-memory',
>    'data': { 'paging': 'bool', 'protocol': 'str', '*begin': 'int',
> -            '*length': 'int' } }
> +            '*length': 'int', '*format': 'DumpGuestMemoryFormat' } }
>  
>  ##
>  # @netdev_add:
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index 02cc815..9158f22 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -791,8 +791,8 @@ EQMP
>  
>      {
>          .name       = "dump-guest-memory",
> -        .args_type  = "paging:b,protocol:s,begin:i?,end:i?",
> -        .params     = "-p protocol [begin] [length]",
> +        .args_type  = "paging:b,protocol:s,begin:i?,end:i?,format:s?",
> +        .params     = "-p protocol [begin] [length] [format]",
>          .help       = "dump guest memory to file",
>          .user_print = monitor_user_noop,
>          .mhandler.cmd_new = qmp_marshal_input_dump_guest_memory,
> @@ -813,6 +813,9 @@ Arguments:
>             with length together (json-int)
>  - "length": the memory size, in bytes. It's optional, and should be specified
>              with begin together (json-int)
> +- "format": the format of guest memory dump. It's optional, and can be
> +            elf|kdump-zlib|kdump-lzo|kdump-snappy, but non-elf formats will
> +            conflict with paging and filter, ie. begin and length(json-string)

Please add a space between "length" and "(json-string)".

Thank you!
Laszlo