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

[Michal Privoznik]

On 07/24/2018 06:19 PM, Clementine Hayat wrote:
> 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
>>


>>> @@ -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?

I think saving that for a separate patch is okay.
Speaking of threads, I forgot to mention, feel free to send v3 as a
completely new thread. We don't really thread versions under v1.

Michal