[libvirt] [PATCH v2 2/3] storage: Introduce iscsi_direct pool type

[Michal Privoznik]

On 07/23/2018 08:43 PM, clem at lse.epita.fr wrote:
> From: Clementine Hayat <clem at lse.epita.fr>
> 
> Introducing the pool as a noop. Integration inside the build
> system. Implementation will be in the following commits.
> 
> Signed-off-by: Clementine Hayat <clem at lse.epita.fr>
> ---
>  configure.ac                               |  6 ++-
>  m4/virt-storage-iscsi-direct.m4            | 41 +++++++++++++++
>  src/conf/domain_conf.c                     |  4 ++
>  src/conf/storage_conf.c                    | 33 ++++++++++--
>  src/conf/storage_conf.h                    |  1 +
>  src/conf/virstorageobj.c                   |  2 +
>  src/storage/Makefile.inc.am                | 22 ++++++++
>  src/storage/storage_backend.c              |  6 +++
>  src/storage/storage_backend_iscsi_direct.c | 58 ++++++++++++++++++++++
>  src/storage/storage_backend_iscsi_direct.h |  6 +++
>  src/storage/storage_driver.c               |  1 +
>  tools/virsh-pool.c                         |  3 ++
>  12 files changed, 178 insertions(+), 5 deletions(-)
>  create mode 100644 m4/virt-storage-iscsi-direct.m4
>  create mode 100644 src/storage/storage_backend_iscsi_direct.c
>  create mode 100644 src/storage/storage_backend_iscsi_direct.h

Missing documentation. I can not push these without any documentation.
You need to document what the new type is doing, what the XML should
look like. Also, might be worth putting some test cases into
storagepoolxml2xmltest.
Since you will be sending v3, can you add docs/news.xml entry (in a
separate patch) too please?

> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 7396616eda..5af27a6ad2 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -30163,6 +30163,10 @@ virDomainDiskTranslateSourcePool(virDomainDiskDefPtr def)
>  
>          break;
>  
> +    case VIR_STORAGE_POOL_ISCSI_DIRECT:
> +        def->src->srcpool->mode = VIR_STORAGE_SOURCE_POOL_MODE_DIRECT;
> +        break;
> +
>      case VIR_STORAGE_POOL_ISCSI:
>          if (def->startupPolicy) {
>              virReportError(VIR_ERR_XML_ERROR, "%s",
> diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c
> index 5036ab9ef8..ee1586410b 100644
> --- a/src/conf/storage_conf.c
> +++ b/src/conf/storage_conf.c
> @@ -62,9 +62,9 @@ VIR_ENUM_IMPL(virStoragePool,
>                VIR_STORAGE_POOL_LAST,
>                "dir", "fs", "netfs",
>                "logical", "disk", "iscsi",
> -              "scsi", "mpath", "rbd",
> -              "sheepdog", "gluster", "zfs",
> -              "vstorage")
> +              "iscsi-direct", "scsi", "mpath",
> +              "rbd", "sheepdog", "gluster",
> +              "zfs", "vstorage")
>  
>  VIR_ENUM_IMPL(virStoragePoolFormatFileSystem,
>                VIR_STORAGE_POOL_FS_LAST,
> @@ -207,6 +207,17 @@ static virStoragePoolTypeInfo poolTypeInfo[] = {
>           .formatToString = virStoragePoolFormatDiskTypeToString,
>        }
>      },
> +    {.poolType = VIR_STORAGE_POOL_ISCSI_DIRECT,
> +     .poolOptions = {
> +         .flags = (VIR_STORAGE_POOL_SOURCE_HOST |
> +                   VIR_STORAGE_POOL_SOURCE_DEVICE |
> +                   VIR_STORAGE_POOL_SOURCE_NETWORK |
> +                   VIR_STORAGE_POOL_SOURCE_INITIATOR_IQN),
> +      },
> +      .volOptions = {
> +         .formatToString = virStoragePoolFormatDiskTypeToString,
> +      }
> +    },
>      {.poolType = VIR_STORAGE_POOL_SCSI,
>       .poolOptions = {
>           .flags = (VIR_STORAGE_POOL_SOURCE_ADAPTER),
> @@ -803,6 +814,19 @@ virStoragePoolDefParseXML(xmlXPathContextPtr ctxt)
>              goto error;
>      }
>  
> +    if (ret->type == VIR_STORAGE_POOL_ISCSI_DIRECT) {
> +        if (!ret->source.initiator.iqn) {
> +            virReportError(VIR_ERR_XML_ERROR, "%s",
> +                           _("missing storage pool initiator iqn"));
> +            goto error;
> +        }
> +        if (!ret->source.ndevice) {
> +            virReportError(VIR_ERR_XML_ERROR, "%s",
> +                           _("missing storage pool device path"));
> +            goto error;
> +        }
> +    }

So the wole idea of poolOptions and volOptions is to specify which parts
of pool/volume XML are required so that we don't have to base checks
like this on ret->type rather than flags.
On the other hand, comment just above VIR_STORAGE_POOL_SOURCE_* enum
says it declares mandatory components which it clearly doesn't. So after
all I think we are safe here.

> +
>   cleanup:
>      VIR_FREE(uuid);
>      VIR_FREE(type);
> @@ -1004,7 +1028,8 @@ virStoragePoolDefFormatBuf(virBufferPtr buf,
>       * files, so they don't have a target */
>      if (def->type != VIR_STORAGE_POOL_RBD &&
>          def->type != VIR_STORAGE_POOL_SHEEPDOG &&
> -        def->type != VIR_STORAGE_POOL_GLUSTER) {
> +        def->type != VIR_STORAGE_POOL_GLUSTER &&
> +        def->type != VIR_STORAGE_POOL_ISCSI_DIRECT) {
>          virBufferAddLit(buf, "<target>\n");
>          virBufferAdjustIndent(buf, 2);

Might be worth updating the comment just above this block ;-)


Michal