Re: [Qemu-devel] [PATCH 2/3] block: adjust qmp_transaction to be extendable

[Kevin Wolf <kwolf@redhat.com>]

Am 03.04.2013 um 11:02 hat Wenchao Xia geschrieben:
> >
> >No, if bdrv_snapshot_delete() can fail, you need to split it in two
> >parts: one that can fail, and one that cannot.  If you cannot, then
> >there are two possibilities:
> >
> >- if the failures are minor and could be repaired with "qemu-img check -r"
> >(e.g. lost clusters), then this is not optimal but can still be done;
> >
> >- otherwise, the operation simply cannot be made transactionable.
> >
> >In the case of qcow2_snapshot_delete, everything except
> >
> >     ret = bdrv_pwrite_sync(bs->file, offsetof(QCowHeader, nb_snapshots),
> >                            &header_data, sizeof(header_data));
> >     if (ret < 0) {
> >         goto fail;
> >     }
> >
> >must be in the prepare phase.  Everything after "fail" (which right now
> >is nothing, but it should at least undo the qcow2_alloc_clusters operation)
> >must be in the rollback phase.  Everything in the middle is the commit
> >phase.
> >
> >Paolo
> >
>   Sorry I haven't state it clearly. What about bdrv_snapshot_create()
> operation? If it need to be rolled back, I think bdrv_snapshot_delete()
> will get called and it may fail.

This means that you're doing it wrong. Instead of deleting the snapshot
on rollback, you shouldn't complete the creation until commit.

If the one bdrv_pwrite_sync() in the commit stage fails, we cannot
maintain transactional semantics. I guess its unlikely enough.

> But in most case if
> bdrv_snapshot_create() succeed before, the bdrv_snapshot_delete should
> succeed also, so if fail there may be unexpected error below, could
> assert be used for this?

assert() is only for programming errors, not for things like hardware
failure.

Kevin