11:10 a.m.
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