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

[Clementine Hayat]

2018-07-24 14:24 GMT+02:00 Michal Privoznik <mprivozn at redhat.com>:
> 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?

Yes sure.

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

That actually isn't the case for the initiator iqn flag.
Is it on purpose or should I patch it in another thread?

>> +
>>   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 ;-)
>

Sure!

-- 
Clementine Hayat