[libvirt] [PATCH 6/7] storage: Process storage pool capabilities

[John Ferlan]

On 1/29/19 5:18 AM, Michal Privoznik wrote:
> On 1/16/19 2:15 AM, John Ferlan wrote:
>> https://bugzilla.redhat.com/show_bug.cgi?id=1581670
>>
>> During storage driver backend initialization, let's save
>> which backends are available in the storage pool capabilities.
>>
>> In order to format those, we need add a connectGetCapabilities
>> processor to the storageHypervisorDriver. This allows a storage
>> connection, such as "storage:///system" to find the API and
>> format the results.
>>
>> NB: This is not available from other hypervisor drivers such as
>> QEMU type connections.
>>
>> Signed-off-by: John Ferlan <jferlan at redhat.com>
>> ---
>>   src/conf/virstorageobj.h      |  4 ++++
>>   src/storage/storage_backend.c | 16 ++++++++++++++++
>>   src/storage/storage_backend.h |  3 +++
>>   src/storage/storage_driver.c  | 17 +++++++++++++++++
>>   4 files changed, 40 insertions(+)
>>
>> diff --git a/src/conf/virstorageobj.h b/src/conf/virstorageobj.h
>> index 1106aa71bd..34adc26df3 100644
>> --- a/src/conf/virstorageobj.h
>> +++ b/src/conf/virstorageobj.h
>> @@ -24,6 +24,8 @@
>>     # include "storage_conf.h"
>>   +# include "capabilities.h"
>> +
>>   typedef struct _virStoragePoolObj virStoragePoolObj;
>>   typedef virStoragePoolObj *virStoragePoolObjPtr;
>>   @@ -45,6 +47,8 @@ struct _virStorageDriverState {
>>         /* Immutable pointer, self-locking APIs */
>>       virObjectEventStatePtr storageEventState;
>> +
>> +    virCapsPtr caps;
> 
> Would be nice to put a comment here:
> /* Immutable pointer, read only after initialized */
> 

Sure.

>>   };
>>     typedef bool
>> diff --git a/src/storage/storage_backend.c
>> b/src/storage/storage_backend.c
>> index a54c338cf0..0ccc616db4 100644
>> --- a/src/storage/storage_backend.c
>> +++ b/src/storage/storage_backend.c
>> @@ -187,3 +187,19 @@ virStorageBackendForType(int type)
>>                      type, NULLSTR(virStoragePoolTypeToString(type)));
>>       return NULL;
>>   }
>> +
>> +
>> +virCapsPtr
>> +virStorageBackendGetCapabilities(void)
>> +{
>> +    virCapsPtr caps;
>> +    size_t i;
>> +
>> +    if (!(caps = virCapabilitiesNew(virArchFromHost(), false, false)))
>> +        return NULL;
>> +
> 
> The fact that you need to pass virArchFromHost() and false, false means,
> that we need to abstract our virCaps even more. I mean, what has the
> host architecture has to do with storage capabilities? Moreover, this is
> how the beginning of storage caps look like:
> 
> # virsh -c storage:///system capabilities
> <capabilities>
> 
>   <host>
>     <cpu>
>       <arch>x86_64</arch>
>     </cpu>
>     <power_management/>
>     <iommu support='no'/>
>   </host>
> 

Yes, I found it odd; however, from your summary comment "This one is
fine on its own, but will need a preceeding patch to abstract virCaps
even more."

How much abstraction do you really^^infinity want? Usage of ->caps.host
is more prevalent than I think it's worth to try and abstract it away
such that it would only be formatted if virCapsHostPtr != NULL. There's
a lot of code that wants to poke around and perhaps know more about
caps.host than should... Certainly more than I'd prefer altering.

Or could it be "simple enough" as modifying virCapabilitiesFormatHostXML
to return early:

    /* Without the presence of some data means we have nothing
     * minimally to format, so just return. */
    if (!virUUIDIsValid(host->host_uuid) &&
        !host->arch && !host->powerMgmt && !host->iommu)
        return 0;

Initially I tried !host->cpu, but that tripped up all the
tests/qemucaps2xmloutdata/caps_*.xml files.

That also means the above call would pass VIR_ARCH_NONE.

>> +    for (i = 0; i < virStorageBackendsCount; i++)
>> +        virCapabilitiesAddStoragePool(caps,
>> virStorageBackends[i]->type);
>> +
>> +    return caps;
>> +}
>> diff --git a/src/storage/storage_backend.h
>> b/src/storage/storage_backend.h
>> index 2b178494ae..c670c66287 100644
>> --- a/src/storage/storage_backend.h
>> +++ b/src/storage/storage_backend.h
>> @@ -126,4 +126,7 @@ int virStorageBackendDriversRegister(bool
>> allmodules);
>>     int virStorageBackendRegister(virStorageBackendPtr backend);
>>   +virCapsPtr
>> +virStorageBackendGetCapabilities(void);
>> +
>>   #endif /* LIBVIRT_STORAGE_BACKEND_H */
>> diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c
>> index 4a13e90481..04beff6b07 100644
>> --- a/src/storage/storage_driver.c
>> +++ b/src/storage/storage_driver.c
>> @@ -300,6 +300,9 @@ storageStateInitialize(bool privileged,
>>         driver->storageEventState = virObjectEventStateNew();
>>   +    if (!(driver->caps = virStorageBackendGetCapabilities()))
>> +        goto error;
>> +
> 
> Initially, I suspected that we want to refresh the caps on every 'virsh
> capabilities' (just like we're doing in say qemu driver). But this is
> different because we wouldn't load a storage driver out of the blue
> (where as admin might install new qemu binaries once we're running).
> Makes sense.
> 

I can add a comment here has well...

Tks -

John