Re: [Qemu-devel] [PATCH 2/6] bitmap dump code via QAPI framework

[Eric Blake <eblake@redhat.com>]

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

On 05/20/2014 11:47 AM, Sanidhya Kashyap wrote:
> This patch introduces the mechanism of dumping the dirty bitmap either
> in an ascii format or binary format. The implementation is almost
> similar to the migration one. A separate thread is created for the
> dumping process.
> 
> The bitmap is obtained with the help of ramlist blocks. I have used almost
> similar functions that have been used in the migration mechanism (in
> arch_init.c file). The mechanism to save the dirty bitmap is based on
> RamBlock rather than MemoryRegion. After having a discussion with Juan, there
> are still some issues with MemoryRegion like
> * memory regions can overlap as well as
> * no port of TCG to MemoryRegion.
> 

> 
> +++ b/include/qapi/qmp/qerror.h
> @@ -118,6 +118,9 @@ void qerror_report_err(Error *err);
>  #define QERR_MIGRATION_ACTIVE \
>      ERROR_CLASS_GENERIC_ERROR, "There's a migration process in progress"
>  
> +#define QERR_LOG_DIRTY_BITMAP_ACTIVE \
> +    ERROR_CLASS_GENERIC_ERROR, "Dirty bitmap dump already in progress"
> +

Please don't add new ERROR_CLASS macros.  Instead, just use error_setg()
and directly output the error message at the call site that produces the
error.

> +++ b/qapi-schema.json
> @@ -4700,3 +4700,13 @@
>                'btn'     : 'InputBtnEvent',
>                'rel'     : 'InputMoveEvent',
>                'abs'     : 'InputMoveEvent' } }
> +##
> +# @log-dirty-bitmap
> +#
> +# provides the dirty bitmap for time t if specified.

Too light on the specification.  You should document all 4 parameters,
and the default value for when the parameters are omitted.

'time t' isn't mentioned in any of the parameter names, so I'm assuming
the docs should mention its significance.

Missing a 'Since: 2.1' designation.

> +##
> +{ 'command' : 'log-dirty-bitmap',
> +  'data'    : { 'filename'      : 'str',
> +                '*epochs'       : 'int',
> +                '*frequency'    : 'int',
> +                '*readable'     : 'bool' } }

Seems okay.  As long as you use qemu_open(), it should also allow
management to pass in an fd with the add-fd mechanism.

> +
> +Arguments:
> +
> +- "filename": name of the file, in which the bitmap will be saved
> +- "epochs": number of times, the memory will be logged

s/times,/times/

> +- "frequency": time difference in milliseconds between each epoch
> +- "readable": dumps the bitmap in hex format (non-binary)

Possibly a poor choice of naming (binary output IS machine-readable; in
fact, programs prefer parsing binary output over decoding human-readable
text); maybe 'pretty' or 'text' would have been better; or even invert
the sense and have it be 'binary' and default to true.  I also wonder if
we even need it - I'd rather have the QMP command always output raw
binary, and then rely on post-processing to turn it into whatever format
the user wants to see (od works wonders), than to bloat qemu with having
to generate a human-readable variant.

> +
> +Examples:
> +-> { "execute" : "log-dirty-bitmap",
> +     "arguments" : {
> +         "filename" : "/tmp/fileXXX",
> +         "epochs" : 100,
> +         "frequency" : 100 } }
> +
> +<- { "return": {} }
> +
> +Note: The epochs, frequency and readable are optional. epochs default
> +value is 3 while that of frequency is 40 and readable defaults to false.

Mention the defaults alongside the description of each parameter, not in
a footnote.


> +static inline void check_epochs_value(int64_t *epochs, Error **errp)
> +{
> +    if (*epochs <= 0) {
> +        error_setg(errp, "epoch size must be greater than 0, setting"
> +                        " a value of 3\n");
> +        *epochs = 3;

This error reporting won't work.  Since this function returns void, if
errp is set at all, the caller must assume failure, rather than hoping
that you replaced the user's bad input with a saner default value.  If
it was worth reporting the error, then it is not worth trying to fix up
the value.

> +/*
> + * copied from arch_init.c file (below function)
> + */
> +
> +static void dirty_bitmap_logging_sync_range(ram_addr_t start,
> +                                            ram_addr_t length)
> +{

Rather than copying a function, can you make the original non-static and
share it between both callers?  Otherwise, the two will get out of sync
if a bug is fixed in one.

> +void qmp_log_dirty_bitmap(const char *filename, bool has_epochs,
> +                          int64_t epochs, bool has_frequency,
> +                          int64_t frequency, bool has_readable,
> +                          bool readable, Error **errp)
> +{
> +    FILE *f;
> +    BitmapLogState *b = logging_current_state();
> +
> +    if (b->state == LOG_BITMAP_STATE_ACTIVE ||
> +            b->state == LOG_BITMAP_STATE_SETUP ||
> +            b->state == LOG_BITMAP_STATE_CANCELING) {
> +        error_set(errp, QERR_LOG_DIRTY_BITMAP_ACTIVE);
> +        return;
> +    }
> +
> +    if (b->state == LOG_BITMAP_STATE_COMPLETED) {
> +        logging_state_set_status(b, LOG_BITMAP_STATE_COMPLETED,
> +                                    LOG_BITMAP_STATE_NONE);
> +    } else if (b->state == LOG_BITMAP_STATE_CANCELLED) {
> +        logging_state_set_status(b, LOG_BITMAP_STATE_CANCELLED,
> +                                    LOG_BITMAP_STATE_NONE);
> +    }
> +
> +    if (readable) {

readable is undefined if has_readable is false.  You are missing
something like:

if (!has_readable) {
    readable = false;
}

> +        f = fopen(filename, "w");
> +    } else {
> +        f = fopen(filename, "wb");

Ouch. You should be using qemu_open, not fopen, so that you can
automatically support things like /dev/fdset/NNN when coupled with the
'add-fd' command for passing open file descriptors.

> +    if (!has_epochs) {
> +        epochs = 3;
> +    }
> +    if (!has_frequency) {
> +        frequency = 40;

Magic numbers; use a named constant.

-- 
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: 604 bytes --]