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.
