Re: [libvirt] [PATCH v2 2/3] storage: Introduce iscsi_direct pool type
On 07/24/2018 06:19 PM, Clementine Hayat wrote:
2018-07-24 14:24 GMT+02:00 Michal Privoznik
<mprivozn(a)redhat.com>:
> On 07/23/2018 08:43 PM, clem(a)lse.epita.fr wrote:
>> From: Clementine Hayat <clem(a)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(a)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