>> +#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
