Jamie Lokier wrote:
> 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).
>
Good question. I looked at some of the formats (qcow, qcow2, vmdk) and didn't
see
anything problematic, since in the close function I didn't see any changes to
the real file,
only in-memory data and memory free.
But an answer from an expert would help.
> 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?
I didn't see any such checking/validation.
It seems that handling such cases will complicate things more than you gain.
>
> 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?
AFAIK, the locks thread terminated, don't think anything committed.
But surely, there's a tight relationship between read-only/locks and sharing.
>
> Thanks!
> -- Jamie
Thanks,
Naphtali
