Re: [libvirt] [PATCH 00/30] storagefile, security: qcow2 data_file support

On Mon, Oct 7, 2019 at 11:49 PM Cole Robinson <crobinso(a)redhat.com&gt; wrote: > > This series is the first steps to teaching libvirt about qcow2 > data_file support, aka external data files or qcow2 external metadata. > > A bit about the feature: it was added in qemu 4.0. It essentially > creates a two part image file: a qcow2 layer that just tracks the > image metadata, and a separate data file which is stores the VM > disk contents. AFAICT the driving use case is to keep a fully coherent > raw disk image on disk, and only use qcow2 as an intermediate metadata > layer when necessary, for things like incremental backup support. > > The original qemu patch posting is here: > https://lists.gnu.org/archive/html/qemu-devel/2019-02/msg07496.html > > For testing, you can create a new qcow2+raw data_file image from an > existing image, like: > > qemu-img convert -O qcow2 \ > -o data_file=NEW.raw,data_file_raw=yes > EXISTING.raw NEW.qcow2 > > The goal of this series is to teach libvirt enough about this case > so that we can correctly relabel the data_file on VM startup/shutdown. > The main functional changes are > > * Teach storagefile how to parse out data_file from the qcow2 header > * Store the raw string as virStorageSource->externalDataStoreRaw > * Track that as its out virStorageSource in externalDataStore > * dac/selinux relabel externalDataStore as needed > > >From libvirt's perspective, externalDataStore is conceptually pretty > close to a backingStore, but the main difference is its read/write > permissions should match its parent image, rather than being readonly > like backingStore. > > This series has only been tested on top of the -blockdev enablement > series, but I don't think it actually interacts with that work at > the moment. > > > Future work: > * Exposing this in the runtime XML. We need to figure out an XML > schema. It will reuse virStorageSource obviously, but the main > thing to figure out is probably 1) what the top element name > should be ('dataFile' maybe?), 2) where it sits in the XML > hierarchy (under <disk> or under <source> I guess) > > * Exposing this on the qemu -blockdev command line. Similar to how > in the blockdev world we are explicitly putting the disk backing > chain on the command line, we can do that for data_file too. Then > like persistent <backingStore> XML the user will have the power > to overwrite the data_file location for an individual VM run. > > * Figure out how we expect ovirt/rhev to be using this at runtime. > Possibly taking a running VM using a raw image, doing blockdev-* > magic to pivot it to qcow2+raw data_file, so it can initiate > incremental backup on top of a previously raw only VM? > > > Known issues: > * In the qemu driver, the qcow2 image metadata is only parsed > in -blockdev world if no <backingStore> is specified in the > persistent XML. So basically if there's a <backingStore> listed, > we never parse the qcow2 header and detect the presence of > data_file. Fixable I'm sure but I didn't look into it much yet. > > Most of this is cleanups and refactorings to simplify the actual > functional changes. > > Cole Robinson (30): > storagefile: Make GetMetadataInternal static > storagefile: qcow1: Check for BACKING_STORE_OK > storagefile: qcow1: Fix check for empty backing file > storagefile: qcow1: Let qcowXGetBackingStore fill in format > storagefile: Check version to determine if qcow2 or not > storagefile: Drop now unused isQCow2 argument > storagefile: Use qcowXGetBackingStore directly > storagefile: Push 'start' into qcow2GetBackingStoreFormat > storagefile: Push extension_end calc to qcow2GetBackingStoreFormat > storagefile: Rename qcow2GetBackingStoreFormat > storagefile: Rename qcow2GetExtensions 'format' argument > storagefile: Fix backing format \0 check > storagefile: Add externalDataStoreRaw member > storagefile: Parse qcow2 external data file > storagefile: Fill in meta->externalDataStoreRaw > storagefile: Don't access backingStoreRaw directly in > FromBackingRelative > storagefile: Split out virStorageSourceNewFromChild > storagefile: Add externalDataStore member > storagefile: Fill in meta->externalDataStore > security: dac: Drop !parent handling in SetImageLabelInternal > security: dac: Add is_toplevel to SetImageLabelInternal > security: dac: Restore image label for externalDataStore > security: dac: break out SetImageLabelRelative > security: dac: Label externalDataStore > security: selinux: Simplify SetImageLabelInternal > security: selinux: Drop !parent handling in SetImageLabelInternal > security: selinux: Add is_toplevel to SetImageLabelInternal > security: selinux: Restore image label for externalDataStore > security: selinux: break out SetImageLabelRelative > security: selinux: Label externalDataStore Hi Cole, it seems the changes to dac/selinux follow a common pattern, in the past those changes then mostly applied to security-apparmor as well. Are you going to add patches for that security backend as well before this is final or do you expect the apparmor users Debian/Suse/Ubuntu to do so later on? Furthermore for the static XML->Apparmor part it will most likely need an extension to [1] as well. Fortunately as you said it is very similar to backingDevices which means it should be rather easy to add this. [1]: https://libvirt.org/git/?p=libvirt.git;a=blob;f=src/security/virt-aa-help...