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

[Juan Quintela <quintela@redhat.com>]

Sanidhya Kashyap <sanidhya.iiith@gmail.com> wrote:
> Following are the changes made with respect to the previous version:
> Chen's advice
> 1) Replaced DIRTY_MEMORY_LOG_BITMAP with DIRTY_MEMORY_MIGRATION and
> completely removed the DIRTY_MEMORY_LOG_BITMAP flag.
>
> Eric's advice
> 2) Replaced FILE pointer with file descriptor.
> 3) Replaced fopen/fclose with qemu_open / qemu_close.
> 4) Removed text format, output only in machine-readable format.
> 5) Defined constants.



> +
> +static inline bool check_value(int64_t value, int64_t min_value,
> +                               const char *str, Error **errp)

If you pass min_value, you should also pass max_value, no?

> +{
> +    if (value < min_value) {
> +        error_setg(errp, "%s's value must be greater than %ld",
> +                         str, min_value);
> +        return false;
> +    }
> +    if (value > LOG_SIZE_MAX) {
> +        error_setg(errp, "%s's value must be less than %ld",
> +                         str, LOG_SIZE_MAX);

You use it for more things that LOG, right?

Name of the function can be changed:

   value_in_range()?


> +    g_free(logging_bitmap);
> +    qemu_close(b->fd);
> +    logging_bitmap = NULL;

Change orders of this line with previous one?

> +    b = NULL;

Why?  b is a local variable.
Do you mean:

	b->fd = -1;?

> +     */
> + log_thread_end:
> +    logging_bitmap_close(b);
> +    if (b->state == LOG_BITMAP_STATE_ACTIVE) {
> +        logging_state_set_status(b, LOG_BITMAP_STATE_ACTIVE,
> +                                    LOG_BITMAP_STATE_COMPLETED);
> +    } else if (b->state == LOG_BITMAP_STATE_CANCELING) {
> +        logging_state_set_status(b, LOG_BITMAP_STATE_CANCELING,
> +                                    LOG_BITMAP_STATE_COMPLETED);
> +    } else if (b->state == LOG_BITMAP_STATE_ERROR) {
> +        logging_state_set_status(b, LOG_BITMAP_STATE_ERROR,
> +                                    LOG_BITMAP_STATE_COMPLETED);
> +    }

can you use a switch here, please?

> +{
> +    int fd = -1;
> +    BitmapLogState *b = logging_current_state();
> +    Error *local_err = NULL;
> +    if (b->state == LOG_BITMAP_STATE_ACTIVE ||
> +            b->state == LOG_BITMAP_STATE_SETUP ||
> +            b->state == LOG_BITMAP_STATE_CANCELING) {
> +        b = NULL;

Not needed.  This is repeated in more places.

> +    b->total_epochs = epochs;
> +    b->current_frequency = frequency;

I would have written it as:

	if (has_epochs) {
             b->total_epochs = epochs;
        } else {
             b->total_epochs IN_EPOCH_VALUE;
        }

The same with current frequency.

Thanks, Juan.