Re: [libvirt] [PATCH 15/n] conf: use common struct in storage volumes

On 03/31/2014 03:59 AM, Peter Krempa wrote: > On 03/30/14 05:38, Eric Blake wrote: >> A fairly smooth transition. And now that domain disks and >> storage volumes share a common struct, it opens the doors for >> a future patch to expose more details in the XML for both >> objects. >> Let's examine how things are prior to patch 14: >> -/* >> - * How the volume appears on the host >> - */ >> -typedef struct _virStorageVolTarget virStorageVolTarget; >> -typedef virStorageVolTarget *virStorageVolTargetPtr; >> -struct _virStorageVolTarget { >> - char *path; >> - int format; /* enum virStorageFileFormat */ both already in virStorageSource >> - virStoragePermsPtr perms; >> - virStorageTimestampsPtr timestamps; Neither in virStorageSource, but present in 'vol-dumpxml' output for both the <target> (primary volume information) and <backingStore> (immediate backing chain information) elements. I personally think it would be useful to have both of these items added to output-only <domain> information - it is NICE to know the timestamps and permissions of a disk image in use by a domain.

>> - int partType; /* enum virStorageVolTypeDisk, only used by disk >> - * backend for partition type */ Not in virStorageSource - this was the field that I'm least sure of what to do. Is it possible to have a partition-based pool whose volumes are even listed with a <backingStore>?

>> - /* The next three are currently only used in vol->target, >> - * not in vol->backingStore. */ >> - virStorageEncryptionPtr encryption; This one is present in virStorageSource, which means that both storage volumes and domain disks care about it. >> - virBitmapPtr features; >> - char *compat; These two are not yet in virStorageSource, but definitely belong there, right alongside driver and format (that is, knowing something is qcow2 is only part of the picture - we really DO want to know qcow2v2 (compat=0.10) vs. qcow2v3 (compat=1.1) when traversing backing chains, particularly since RHEL 6 still lacks qcow2v3 support). >> - virStorageVolTarget target; >> - virStorageVolTarget backingStore; >> + virStorageSource target; > > Hmmm, converting target to a source definition doesn't seem to be a good > idea to me: > > * As in other parts of the code, the semantics of the target and source > differ in the data needed to store about those fields. Only in that the code _currently_ displays more for <target> than it does for <backingStore>, but the two are otherwise highly related. Also, I envision making a storage volume <backingStore> recursive, just as I do for domain <disk> backing store information. Knowing only the first backing element is not as useful as knowing the entire backing chain. > > * Tracking of the target requires us to add additional data to the > struct which makes it to bloat and it's probable that those metadata > won't be used by other parts of the code. You may have a point for partType, but the other parts of the metadata ARE useful in other parts of the code, even if we aren't using them yet. > > * Other parts that are already present in the source structure are > irrelevant to the host representation, such as the network information. Not irrelevant, so much as redundant. That is, when listing a storage volume from a network storage pool such as gluster, the network information can be obtained from the pool that owns the storage volume - but it shouldn't hurt to also list the network information in the storage volume itself instead of forcing callers to make multiple API calls to obtain all the pieces. > > I'd rather see those separated. It might seem that the structs carry > similar data but they are semantically different IMO. > >> + virStorageSource backingStore; > > In case of the backing chain I think that the backing store is a > property of the source and not the target thus I'm fine with changing this. The name <target> in storage volume XML is rather misleading. It really is the same as the <source> element of a domain <disk> element. The name <source> in storage volume XML gives additional details (such as extent mappings on things such as LVM volumes), but THAT is the one that is semantically unrelated to <source> in a domain <disk> element. I can indeed limit the patch to just changing the backingStore to the common type, but I'm not yet convinced why changing the <target> to use virStorageSource is wrong.