Re: [Qemu-devel] [PATCH 3/4] Block: readonly changes

[Jamie Lokier <jamie@shareable.org>]

Naphtali Sprei wrote:
> Open backing file for read-only
> During commit upgrade to read-write and back at end to read-only

> +    if (ro) { /* re-open as RO */
> +        bs_ro = bdrv_new("");
> +        ret = bdrv_open2(bs_ro, bs->backing_hd->filename,  bs->backing_hd->open_flags & ~BDRV_O_RDWR, NULL);
> +        if (ret < 0) {
> +            bdrv_delete(bs_ro);
> +            return -EACCES;
> +        }
> +        bdrv_close(bs->backing_hd);
> +        qemu_free(bs->backing_hd);
> +        bs->backing_hd = bs_ro;
> +        bs->backing_hd->keep_read_only = 0;
> +    }

I think the general idea is perfect.

A couple of concerns come to mind.

1. When changing read-write to read-only, if the backing file is a complex
   format like qcow2 (or any others), is it possible for this bdrv_open2()
   to read metadata such as format indexes, and even data, _before_
   all changes maintained by bs->backing_hd have been written to the file?

   (If the complex formats were like real filesystems and had a "mounted"
   flags as real filesystems tend to, then it would be an issue, but I'm
   not aware of any of them doing that.)

   Are there any such issues when switching from read-only to read-write
   earlier?  (It seems unlikely).

2. Secondly, what if the re-open gets a different file (testable with
   fstat()).  I know, you get what you deserve if you rename files, but
   still, do any of the formats which use backing files have a UUID check
   or something to confirm they are using the right backing file, which
   might be subverted by this?

3. What about the bdrv file/device-locking which was actively looking
   like it might get in a couple of months ago.  Did it get in, and if
   so does it conflict with this upgrade pattern?

Thanks!
-- Jamie