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

[Peter Krempa]

On 03/31/14 18:10, Eric Blake wrote:
> 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.
> 

Okay, fair enough in this case. If we will use this it might be worth
adding information about selinux too. (In the future, not right now)

>>> -    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>?

Not sure if libvirt allows and reports it but it's certainly possible to
store a qcow2 overlay in a partition. Thus we might need to be able to
describe that.

> 
>>> -    /* 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.
> 
> 


Well you've convinced me. ACK to the original patch if you well-document
what partType is used for.

PEter

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