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