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

Thu Oct 10 16:22:18 UTC 2019

Hi,

ACKed basically everything, perhaps one or two patches I found something
worth talking about but nothing too gamebreaker. Logic-wise everything made
sense to me, but I believe someone else with a deeper understanding of the
storage backend in Libvirt might know better.

I am not sure how hard it is to test the changes you're making here, but it
would be good to have new stuff in virstoragetest.c (seems like the best 
place)
to cover this new data_file support as well.

On a side note: from patches 20+ I got an impression that I was reviewing
the same patches over and over again. I think it's a good idea to have a 
look
at the code repetition between the files in src/security dir, or at the 
very least
between security_dac.c and security_selinux.c files.

Thanks,

DHB

On 10/7/19 6:49 PM, Cole Robinson 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
>
>   src/libvirt_private.syms        |   1 -
>   src/security/security_dac.c     |  63 +++++--
>   src/security/security_selinux.c |  97 +++++++----
>   src/util/virstoragefile.c       | 281 ++++++++++++++++++++------------
>   src/util/virstoragefile.h       |  11 +-
>   5 files changed, 290 insertions(+), 163 deletions(-)
>