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
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list