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

[Eric Blake]

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 604 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20140331/15305673/attachment-0001.sig>