Re: [Qemu-devel] [v1 Patch 4/10]Qemu: Framework for reopening image files safely

[Eric Blake]

On 06/15/2012 02:47 PM, Supriya Kannery wrote:
> Struct BDRVReopenState along with three reopen related functions
> introduced for handling reopening of images safely. This can be
> extended by each of the block drivers to reopen respective
> image files.
> 
> Signed-off-by: Supriya Kannery <address@hidden>
> 

> +    /* Use driver specific reopen() if available */
> +    if (drv->bdrv_reopen_prepare) {
> +        ret = bdrv_reopen_prepare(bs, &reopen_state, bdrv_flags);
> +         if (ret < 0) {
> +            bdrv_reopen_abort(bs, reopen_state);
> +            qerror_report(QERR_OPEN_FILE_FAILED, bs->filename);
> +            return ret;
> +        }
> +
> +        bdrv_reopen_commit(bs, reopen_state);
> +        bs->open_flags = bdrv_flags;

Good.

> +
> +    } else {
> +
> +        /* Try reopening the image using protocol or directly */
> +        if (bs->file) {
> +            open_flags = bs->open_flags;
> +            drv->bdrv_close(bs);
> +            if (drv->bdrv_file_open) {
> +                ret = drv->bdrv_file_open(bs, bs->filename, bdrv_flags);

Not so good.  Are we sure we can't flush all data, so that we can then
attempt the open before the close, and avoid losing the original file on
error?

> +            } else {
> +                ret = bdrv_file_open(&bs->file, bs->filename, bdrv_flags);
> +            }
> +            if (ret < 0) {
> +                /* Reopen failed. Try to open with original flags */
> +                qerror_report(QERR_OPEN_FILE_FAILED, bs->filename);
> +                if (drv->bdrv_file_open) {
> +                    ret = drv->bdrv_file_open(bs, bs->filename, open_flags);
> +                } else {
> +                    ret = bdrv_file_open(&bs->file, bs->filename, 
> open_flags);
> +                }
> +                if (ret < 0) {
> +                    /* Reopen failed with orig and modified flags */
> +                    bdrv_delete(bs->file);
> +                    bs->drv = NULL;

You dropped the abort() here, but now the device is silently gone.  But
since the error of QERR_OPEN_FILE_FAILED is the same as in the safe
case, how does the management app know that things were corrupted?

-- 
Eric Blake   address@hidden    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature