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

[Michal Privoznik <mprivozn@xxxxxxxxxx>]

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@xxxxxxxxxx>
> ---
>   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 */

>   };
>   
>   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>

> +    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.


Michal

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list