On Mon, Jan 25, 2021 at 10:51:09AM +0100, Peter Krempa wrote:
On Mon, Jan 25, 2021 at 09:33:31 +0000, Daniel Berrange wrote:
On Sat, Jan 23, 2021 at 11:54:51AM +0100, Peter Krempa wrote:
On Fri, Jan 22, 2021 at 20:10:59 -0500, Masayoshi Mizuma wrote:
From: Masayoshi Mizuma <m.mizuma@jp.fujitsu.com>
[...]
Please note that this is a high level review. I've spotted some coding style inconsistencies and such, but since this will need significant rework I'll not point them out.
Also don't forget to add test cases, where it will be visible that the disk (neither frontend nor backend) is added on the commandline.
Also it should be fairly trivial to support it for SCSI disks since they support hotplug too.
Is there a reason we can't use the existing <readonly/> element as a way to make the underlying base image be treated as read only and thus become sharable ?
The problem is that the qemu device frontend code infers the readonly state from the backend image specification when it's instantiated.
Thus if you start qemu with the topmost layer marked as read-only so that the write lock doesn't get asserted, this fact gets stored in the frontend. If you then later insert a writable layer into the chain on top of it, the frontend stays read-only.
IDE/SATA disks are outhright refused to be read-only by qemu, virtio disks remember that they are read-only and keep it into guest execution and SCSI disks caused an abort() (which was recently fixed in qemu upstream.)
Thus the only two options are:
1) hotplug the frontend after we do the shenanigans with the image 2) implement explicit control of the read-only state of the frontend in qemu in the first place, adapt to it in libvirt and then keep using the current approach in libvirt
I must be missing something here, because the topmost layer with <transient> isn't read-only - it would be writable as that's the whole point of the throwaway transient overlay file. It just feels to me that the situation desired is already one that is supported and working when explicitly top+base are specified in the XML. Consider if we have <disk> <source file="top.qcow2> <backingStore> <source file="base.qcow2"> </backing> </disk> IIUC, base.qcow2 will be readonly, and top.qcow2 will be writable by default. I'm suggesting that <disk> <source file="base.qcow2"> <transient/> <readonly/> </disk> should be treated as identical to the first. During domain startup preparation, we would effectvely transform the latter into the former, with a random tempfile for top.qcow. This shouldn't add any real complexity to our existing code and not involve tricks with hotplug. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|