On Tue, Feb 25, 2020 at 12:50:21 +0000, Daniel Berrange wrote: > On Mon, Feb 24, 2020 at 06:10:46PM +0100, Peter Krempa wrote: > > On Mon, Feb 24, 2020 at 14:24:15 +0000, Daniel Berrange wrote: > > > On Mon, Feb 24, 2020 at 02:34:16PM +0100, Peter Krempa wrote: [...] > > > Would there be any downsides to this that did not already exist in the > > > non-blockdev days ? > > > > We can, but the price is that: > > 1) we won't allow blockjobs and anything blockdev-related because node > > name would be out of our control. This was possible in pre-blockdev era. > > Ok, that's not viable then. We can't switch one regression for a different > regression. > > > 2) we will lose control of actually telling qemu to NOT open the backing > > file in that case. Distros using only unix permission still have > > arbitrary file access under permissions of the qemu process. > > True, but that is at least historically expected behaviour, which can > be fixed by setting <backingfile/> in the XML file IIUC. Yes, but if you specify any <backingStore> including the terminator you basically configure the image chain yourselves. The part which is configured explicitly will not undergo any form of detection, not even looking for the backing file. > > 3) weird special-case code, because we need to keep some metadata about > > the image to do security labelling > > > > > I don't think we can solve the regressions in behaviour of backing files > > > by doing probing of the backing files in libvirt, because that only works > > > for the case where libvirt can actually open the file. ie a local file on > > > disk. We don't have logic for opening backing files on RBD, GlusterFS, > > > iSCSI, HTTP, SSH, etc, and nor do we want todo that. > > > > Now we are back in the teritory where we actually do match what would > > happen with previously. We don't specify these on the command line with > > ehaviour matching what's described above, with the caveats as above. > > > > I kept this behaviour because we couldn't do better. This is in place > > even now if the last introspectable image has valid format specified. > > > > We can reconsider how to approach this but ideally separately. > > I'm a little lost as to exactly which scenarios are broken, and which > we're fixing. > > 1. file:top.qcow2 -> file:base.raw > > 2. file:top.qcow2 -> file:base.qcow2 > > 3. file:top.qcow2 -> file:middle.qcow2 -> file:base.raw > > 4. file:top.qcow2 -> file:middle.qcow2 -> file:base.qcow2 I assume you meant that none of the 'qcow2' files have format of the backing file recorded in the metadata. > IIUC, (1) is working before and now, (2) is working before but > broken now, and (3) and (4) were broken before and now. 1) was working before, but is now forbidden 2) was working before, and is now forbidden 3,4) were possibly working (without sVirt), or would get permission denied reported by qemu during startup, currently they are forbidden by a libvirt error message After this patch: 1) will be fixed by this patch 2) will be fixed by this patch 3, 4) will report a libvirt error rather than relying on sVirt or others > So (2) is the only one we /must/ to fix > > Am I right that by doing probing in libvirt as per this patch, > as well as fixing (2) though, we'll accidentally fix (3) and (4) > even though they were always broken before ? > > This all talks about qcow2 images on the file: protocol driver. > What is the situation for, say, the iscsi: protocol driver > > > 1. iscsi:top.qcow2 -> iscsi:base.raw > > 2. iscsi:top.qcow2 -> iscsi:base.qcow2 > > 3. iscsi:top.qcow2 -> iscsi:middle.qcow2 -> iscsi:base.raw > > 4. iscsi:top.qcow2 -> iscsi:middle.qcow2 -> iscsi:base.qcow2 > > What was the behaviour of this in the pre-blockdev days and > vs current git master ? Is it the same with (1) is working > before and now, (2) is working before but broken now, and > (3) and (4) were broken before and now. Pre-blockdev and post-blockdev every scenario of the above is working. We can't even inspect the top file for backing store so everything is just ignored. Now what partially changes is when we have something introspectable in the way: 1,2) file:top.qcow2 -> iscsi:base.whatever The current code would report an error if the format was not recorded in the overlay (top.qcow2) and we can't introspect the backing file we'll reject it. This can be relaxed though. 3,4) will work if top.qcow2 has the format but any subsequent don't since we don't introspect it > I'm assuming the libvirt probing cannot fix any case other > than file: protocol, or host-device: protocol, since we're > unable to open any other type of storage. It fixes also gluster since the backends for that are already implemented. I'll post another rebased version with some updated docs and one fix. We can continue there perhaps.