Sanidhya Kashyap <sanidhya.ii...@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.
