2018-07-24 14:24 GMT+02:00 Michal Privoznik <mprivozn@xxxxxxxxxx>:
> On 07/23/2018 08:43 PM, clem@xxxxxxxxxxxx wrote:
>> From: Clementine Hayat <clem@xxxxxxxxxxxx>
>>
>> Introducing the pool as a noop. Integration inside the build
>> system. Implementation will be in the following commits.
>>
>> Signed-off-by: Clementine Hayat <clem@xxxxxxxxxxxx>
>> ---
>> 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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
