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. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org