Re: [Qemu-devel] [RFC PATCH v9 14/23] replay: checkpoints

[Paolo Bonzini]

On 18/02/2015 12:57, Pavel Dovgalyuk wrote:
> 
> +
> +    if (replay_file) {
> +        if (replay_mode == REPLAY_MODE_PLAY) {
> +            replay_mutex_lock();
> +            if (!skip_async_events(EVENT_CHECKPOINT + checkpoint)) {
> +                if (replay_data_kind == EVENT_ASYNC) {
> +                    replay_read_events(checkpoint);
> +                    replay_fetch_data_kind();
> +                    res = replay_data_kind != EVENT_ASYNC;
> +                    replay_mutex_unlock();
> +                    return res;
> +                }
> +                replay_mutex_unlock();
> +                return res;
> +            }
> +            replay_has_unread_data = 0;
> +            replay_read_events(checkpoint);
> +            replay_fetch_data_kind();
> +            res = replay_data_kind != EVENT_ASYNC;
> +            replay_mutex_unlock();
> +            return res;
> +        } else if (replay_mode == REPLAY_MODE_RECORD) {
> +            replay_mutex_lock();
> +            replay_put_event(EVENT_CHECKPOINT + checkpoint);
> +            replay_save_events(checkpoint);
> +            replay_mutex_unlock();
> +        }
> +    }
> +
> +    return true;

This is cleaner:

    if (!replay_file) {
        return true;
    }

    replay_mutex_lock();

    if (replay_mode == REPLAY_MODE_PLAY) {
        if (skip_async_events(EVENT_CHECKPOINT + checkpoint)) {
            replay_has_unread_data = 0;
        } else if (replay_data_kind != EVENT_ASYNC) {
            res = false;
            goto out;
        }
        replay_read_events(checkpoint);
        replay_fetch_data_kind();
        res = replay_data_kind != EVENT_ASYNC;
    } else if (replay_mode == REPLAY_MODE_RECORD) {
        replay_put_event(EVENT_CHECKPOINT + checkpoint);
        replay_save_events(checkpoint);
        res = true;
    }
out:
    replay_mutex_unlock();
    return res;

I'm not sure however of the meaning of "res = replay_data_kind !=
EVENT_ASYNC;".  Why not just return true since skip_async_events has
returned true?  Can you add a comment?

Paolo


> +        if (!replay_checkpoint(CHECKPOINT_BDRV_DRAIN)) {
> +            /* Do not wait anymore, we stopped at some place in
> +               the middle of execution during replay */
> +            return;
> +        }
>          busy = false;
>  
>          QTAILQ_FOREACH(bs, &bdrv_states, device_list) {
> @@ -2004,6 +2010,12 @@ void bdrv_drain_all(void)
>              aio_context_release(aio_context);
>          }
>      }
> +    if (replay_mode == REPLAY_MODE_PLAY) {
> +        /* Skip checkpoints from the log */
> +        while (replay_checkpoint(CHECKPOINT_BDRV_DRAIN)) {
> +            /* Nothing */
> +        }
> +    }
>  }

Can you explain this?  Otherwise the patch looks great.