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

[Cole Robinson]

On 10/8/19 1:52 AM, Christian Ehrhardt wrote:
> On Mon, Oct 7, 2019 at 11:49 PM Cole Robinson <crobinso at redhat.com> 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-helper.c;h=5853ad985fe17c91af3f1dc39d179f22a1dca5b7;hb=HEAD#l980

I forgot about apparmor, sorry. I just sent a series and CCd you that 
does some apparmor cleanups and tweaks so that for this series the 
data_file coverage is just this extra diff:

diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c
index d9f6b5638b..fc095d2964 100644
--- a/src/security/virt-aa-helper.c
+++ b/src/security/virt-aa-helper.c
@@ -948,6 +948,10 @@ storage_source_add_files(virStorageSourcePtr src,
          if (add_file_path(tmp, depth, buf) < 0)
              return -1;

+        if (src->externalDataStore &&
+            storage_source_add_files(src->externalDataStore, buf, 
depth) < 0)
+            return -1;
+
          depth++;
      }

I will add a proper patch for that when the prep work hits git

Thanks,
Cole