Il 30/09/2013 14:02, Fam Zheng ha scritto: > + /* Mirror code already swapped bs and base, we drop the bs loop chain > + * formed by the swap: break the loop chain, trigger the chain unref. > + */ > + p = info->base->backing_hd; > + info->base->backing_hd = NULL; > + bdrv_unref(p);
Should this code be in mirror_run? Perhaps extracting it into a new function together with the preceding lines: if (bdrv_get_flags(s->target) != bdrv_get_flags(s->common.bs)) { bdrv_reopen(s->target, bdrv_get_flags(s->common.bs), NULL); } bdrv_swap(s->target, s->common.bs); Same for this: > + if (bdrv_reopen(base_bs, bs->open_flags, &local_err)) { > + error_propagate(errp, local_err); > + return; > + } > + bdrv_ref(base_bs); Perhaps you can have an internal function mirror_start_job and two front-ends mirror_start + commit_active_start that wrap it. Similarly, mirror_start_job can take the base BDS and a bool instead of MirrorSyncMode (removing the need for MIRROR_SYNC_MODE_COMMON). But apart from these details, the series looks very nice. Paolo