Re: [Qemu-devel] [PATCH v4 3/8] BitmapLog: bitmap dump code via QAPI framework with runstates

[Sanidhya Kashyap]

>> +#define RAMBLOCK_NAME_LENGTH (1<<8)
> 
> Be careful; making this bigger would break migration formats,
> making it smaller would probably break migration loading.
>

its the same as previous, will write it explicitly. What I think is that
we should make name_length to be global. I have seen at most of the
places that we are using 256 as length, why can't we use a variable?

>> +
>> +enum {
>> +    LOG_BITMAP_STATE_ERROR = -1,
>> +    LOG_BITMAP_STATE_NONE,
>> +    LOG_BITMAP_STATE_SETUP,
>> +    LOG_BITMAP_STATE_ACTIVE,
>> +    LOG_BITMAP_STATE_CANCELING,
>> +    LOG_BITMAP_STATE_COMPLETED
>> +};
> 
> I'd be tempted to give that enum a name and use it as the type
> for functions that pass the state around (although I realise
> you have to be careful with the variable having to be an int
> for the atomic).
>

will see what can be done.


>> +static inline void logging_lock(void)
>> +{
>> +    qemu_mutex_lock_iothread();
>> +    qemu_mutex_lock_ramlist();
>> +}
> 
> I wonder how often you can really not have the ramlist locked; if stuff
> is added/removed the last_ram_offset would change, so to really be safe
> you probably need to hold it for much longer than you currently do -
> but that might not be practical.
> 

I didn't realize that. I will try to make suitable changes in my code to
cater to those added/removed last_ram_offset.

>> +static inline void logging_bitmap_set_dirty(ram_addr_t addr)
>> +{
>> +    int nr  = addr >> TARGET_PAGE_BITS;
> 
> Be careful; int is too small; long is probably what's
> needed (which I think is the type of the parameter to set_bit).
> 

My mistake. I will rectify it!

>> +static inline bool value_in_range(int64_t value, int64_t min_value,
>> +                                  int64_t max_value, const char *str,
>> +                                  Error **errp)
>> +{
>> +    if (value < min_value) {
>> +        error_setg(errp, "%s's value must be greater than %ld",
>> +                         str, min_value);
>> +        return false;
>> +    }
>> +    if (value > max_value) {
>> +        error_setg(errp, "%s's value must be less than %ld",
>> +                         str, max_value);
>> +        return false;
>> +    }
>> +    return true;
>> +}
> 
> This seems a pretty generic function; could it go somewhere
> in util or the like?
> 
okay.

>> +
>> +static void dirty_bitmap_sync(void)
>> +{
>> +    RAMBlock *block;
>> +    address_space_sync_dirty_bitmap(&address_space_memory);
>> +    QTAILQ_FOREACH(block, &ram_list.blocks, next) {
>> +        qemu_bitmap_sync_range(block->mr->ram_addr, block->length,
>> +                               logging_bitmap, false);
>> +    }
>> +}
> 
> I think that should be logging_dirty_bitmap_sync since it's
> specific to logging_bitmap.
> 

okay. Will change it.

>> +
>> +static void logging_state_update_status(BitmapLogState *b)
>> +{
>> +    switch (b->state) {
>> +    case LOG_BITMAP_STATE_ACTIVE:
>> +        logging_state_set_status(b, LOG_BITMAP_STATE_ACTIVE,
>> +                                    LOG_BITMAP_STATE_COMPLETED);
>> +        return;
>> +    case LOG_BITMAP_STATE_CANCELING:
>> +        logging_state_set_status(b, LOG_BITMAP_STATE_CANCELING,
>> +                                    LOG_BITMAP_STATE_COMPLETED);
>> +        return;
>> +    case LOG_BITMAP_STATE_ERROR:
>> +        logging_state_set_status(b, LOG_BITMAP_STATE_ERROR,
>> +                                    LOG_BITMAP_STATE_COMPLETED);
>> +    }
>> +    return;
>> +}
> 
> I didn't really see the point of this at first, but I guess
> it always moves to 'COMPLETED' unless you're already at completed
> or in NONE; but then perhaps:
> 
>     int s = b->state;
>     switch (s) {
>     case LOG_BITMAP_STATE_ACTIVE:
>     case LOG_BITMAP_STATE_CANCELING:
>     case LOG_BITMAP_STATE_ERROR:
>        logging_state_set_status(b, s,
>                                    LOG_BITMAP_STATE_COMPLETED);
>        return;
>    }
>    return;
> 
> would be more obvious (note I only read the state once)
> 

Yup, will save some code and reduce confusion.

-- 
-----

Sanidhya Kashyap