Re: [Qemu-devel] [PATCH v5 2/6] BitmapLog: bitmap dump code

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 4443 bytes --]

On 07/31/2014 09:12 PM, Sanidhya Kashyap wrote:
> In this patch, I have incorporated an enum named QemuProcess
> which defines what kind of process is being executed i.e.
> none --> no other process except the VM execution
> migration --> migration is being executed
> bitmap-dump --> bitmap dump process is undergoing
> 
> Besides this, I have tried to incorporate the dynamic change of
> the last_ram_offset, if it gets change. The downside is that I am
> holding lock for a longer period of time and I don't know whether
> that should be done or not. I am also doing some allocation when
> locked.
> I am not sure whether last_ram_offset gets changed when a device
> is hot plugged or hot unplugged.
> 
> I have modified the variables name as:
> current-iteration: for the current iteration under process
> iterations: total iterations that will be done which is constant
> period: the delay in each iteration.
> 
> Signed-off-by: Sanidhya Kashyap <sanidhya.iiith@gmail.com>
> ---
>  hmp-commands.hx         |  16 ++
>  hmp.c                   |  18 +++
>  hmp.h                   |   1 +
>  include/exec/cpu-all.h  |   5 +-
>  include/sysemu/sysemu.h |   5 +
>  migration.c             |  12 ++
>  qapi-schema.json        |  35 +++++
>  qmp-commands.hx         |  34 +++++
>  savevm.c                | 378 ++++++++++++++++++++++++++++++++++++++++++++++++
>  vl.c                    |  24 +++
>  10 files changed, 527 insertions(+), 1 deletion(-)
> 

> +++ b/hmp-commands.hx
> @@ -1788,6 +1788,22 @@ STEXI
>  show available trace events and their state
>  ETEXI
>  
> +     {
> +        .name       = "ldb|log_dirty_bitmap",

The only other instances of a .name with an | are for giving a
single-letter option name synonym to a long option, but ldb is not a
single-letter option.  I don't think you can create 'ldb' in the HMP
interface.  Just stick with the long name.

> +        .args_type  = "filename:s,iterations:i?,period:i?",
> +        .params     = "filename iterations period",
> +        .help       = "dumps the memory's dirty bitmap to file\n\t\t\t"
> +                      "filename: name of the file in which the bitmap will be saved\n\t\t\t"
> +                      "iterations: number of times, the memory will be logged\n\t\t\t"

s/times,/times/

> +++ b/qapi-schema.json
> @@ -3480,3 +3480,38 @@
>  # Since: 2.1
>  ##
>  { 'command': 'rtc-reset-reinjection' }
> +
> +##
> +# QemuProcess

I agree with David's advice to name this QemuBitmapUser.


> +##
> +# @log-dirty-bitmap
> +#
> +# This command will dump the dirty bitmap to a file by logging the
> +# memory for a specified number of times with a defined time difference
> +#
> +# @filename: name of the file in which the bitmap will be saved.
> +#
> +# @iterations: number of times the memory will be logged (optional). The
> +# and max values are 3 and 100000 respectively.

s/and/min and/

For consistency, the optional markup should look like:

@iterations: #optional number of times the memory will be logged.

Also, you should document what value is used when the option is omitted.

> +#
> +# @period: time difference in milliseconds between each iteration (optional).
> +# The min and max values are 10 and 100000 respectively.

Again, wrong markup for #optional, and missing the default value.


> +Arguments:
> +
> +- "filename": name of the file, in which the bitmap will be saved.
> +
> +- "iterations": number of times, the memory will be logged (optional).
> +  The min and max values are 3 and 100000 respectively.
> +
> +- "period": time difference in milliseconds between each iteration (optional).
> +   The min and max values are 10 and 100000 respectively.
> +

[1]

> +Examples:
> +-> { "execute": "log-dirty-bitmap",
> +     "arguments": {
> +         "filename": "/tmp/fileXXX",
> +         "iterations": 3,
> +         "period": 10 } }
> +
> +<- { "return": {} }
>  
> +Note: The iterations, and period parameters are optional. iterations default
> +value is 3 while that of period is 10.

This note is redundant, and it is not good to split information.
Instead, drop the note paragraph, and mention the optional values of 3
and 10 up above at the point [1] where you mention the arguments are
optional.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 539 bytes --]