On Sat, Jul 14, 2018 at 12:06:14AM +0200, clem@lse.epita.fr wrote:
From: Clementine Hayat <clem@lse.epita.fr>
I know that this is more like RFC patch series so before we get to the actual patch which should be pushed into upstream there should be some commit message.
Signed-off-by: Clementine Hayat <clem@lse.epita.fr> --- configure.ac | 6 ++- m4/virt-storage-iscsi-direct.m4 | 41 +++++++++++++++++++++ src/conf/domain_conf.c | 1 + src/conf/storage_conf.c | 31 ++++++++++++++-- src/conf/storage_conf.h | 1 + src/conf/virstorageobj.c | 2 + src/storage/Makefile.inc.am | 21 +++++++++++ src/storage/storage_backend.c | 6 +++ src/storage/storage_backend_iscsi_direct.c | 43 ++++++++++++++++++++++ src/storage/storage_backend_iscsi_direct.h | 6 +++ src/storage/storage_driver.c | 1 + tools/virsh-pool.c | 3 ++ 12 files changed, 157 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
[...]
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 7396616eda..0a9509de0b 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -30163,6 +30163,7 @@ virDomainDiskTranslateSourcePool(virDomainDiskDefPtr def)
break;
+ case VIR_STORAGE_POOL_ISCSI_DIRECT: case VIR_STORAGE_POOL_ISCSI: if (def->startupPolicy) { virReportError(VIR_ERR_XML_ERROR, "%s",
This will not be good enough. We need to set the default def->src->srcpool->mode to VIR_STORAGE_SOURCE_POOL_MODE_DIRECT if the storage pool is "iscsi-direct" and if the mode is already configured in domain XML we need to check whether it's "direct" mode if the storage pool is "iscsi-direct".
diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index 5036ab9ef8..7a4b00ad8c 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,16 @@ static virStoragePoolTypeInfo poolTypeInfo[] = { .formatToString = virStoragePoolFormatDiskTypeToString, } }, + {.poolType = VIR_STORAGE_POOL_ISCSI_DIRECT, + .poolOptions = { + .flags = (VIR_STORAGE_POOL_SOURCE_HOST | + VIR_STORAGE_POOL_SOURCE_NETWORK | + VIR_STORAGE_POOL_SOURCE_INITIATOR_IQN),
We need to use VIR_STORAGE_POOL_SOURCE_DEVICE as well, otherwise it would not be formatted back.
+ }, + .volOptions = { + .formatToString = virStoragePoolFormatDiskTypeToString, + } + }, {.poolType = VIR_STORAGE_POOL_SCSI, .poolOptions = { .flags = (VIR_STORAGE_POOL_SOURCE_ADAPTER), @@ -802,6 +812,18 @@ virStoragePoolDefParseXML(xmlXPathContextPtr ctxt) "./target/permissions") < 0) goto error; }
One empty line would be nice.
+ 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; + } + }
cleanup: VIR_FREE(uuid); @@ -1004,7 +1026,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);
diff --git a/src/conf/storage_conf.h b/src/conf/storage_conf.h index 15dfd8becf..858623783d 100644 --- a/src/conf/storage_conf.h +++ b/src/conf/storage_conf.h @@ -85,6 +85,7 @@ typedef enum { VIR_STORAGE_POOL_LOGICAL, /* Logical volume groups / volumes */ VIR_STORAGE_POOL_DISK, /* Disk partitions */ VIR_STORAGE_POOL_ISCSI, /* iSCSI targets */ + VIR_STORAGE_POOL_ISCSI_DIRECT, /* iSCSI targets using libiscsi */ VIR_STORAGE_POOL_SCSI, /* SCSI HBA */ VIR_STORAGE_POOL_MPATH, /* Multipath devices */ VIR_STORAGE_POOL_RBD, /* RADOS Block Device */
[...]
diff --git a/src/storage/Makefile.inc.am b/src/storage/Makefile.inc.am index ea98c0ee52..d81864f5b9 100644 --- a/src/storage/Makefile.inc.am +++ b/src/storage/Makefile.inc.am @@ -31,6 +31,11 @@ STORAGE_DRIVER_ISCSI_SOURCES = \ storage/storage_backend_iscsi.c \ $(NULL)
+STORAGE_DRIVER_ISCSI_DIRECT_SOURCES = \ + storage/storage_backend_iscsi_direct.h \ + storage/storage_backend_iscsi_direct.c \ + $(NULL) + STORAGE_DRIVER_SCSI_SOURCES = \ storage/storage_backend_scsi.h \ storage/storage_backend_scsi.c \ @@ -89,6 +94,7 @@ EXTRA_DIST += \ $(STORAGE_FILE_FS_SOURCES) \ $(STORAGE_DRIVER_LVM_SOURCES) \ $(STORAGE_DRIVER_ISCSI_SOURCES) \ + $(STORAGE_DRIVER_ISCSI_DIRECT_SOURCES) \ $(STORAGE_DRIVER_SCSI_SOURCES) \ $(STORAGE_DRIVER_MPATH_SOURCES) \ $(STORAGE_DRIVER_DISK_SOURCES) \ @@ -193,6 +199,21 @@ libvirt_storage_backend_iscsi_la_LIBADD = \ $(NULL) endif WITH_STORAGE_ISCSI
+if WITH_STORAGE_ISCSI_DIRECT +libvirt_storage_backend_iscsi_direct_la_SOURCES = $(STORAGE_DRIVER_ISCSI_DIRECT_SOURCES) +libvirt_storage_backend_iscsi_direct_la_CFLAGS = \
So I remember point out that we don't need '-I$(srcdir)/conf \' here right now but I was wrong. We need it because some files from that directory are indirectly included.
+ $(LIBISCSI_CFLAGS) \ + $(AM_CFLAGS) \ + $(NULL) + +storagebackend_LTLIBRARIES += libvirt_storage_backend_iscsi-direct.la +libvirt_storage_backend_iscsi_direct_la_LDFLAGS = $(AM_LDFLAGS_MOD) +libvirt_storage_backend_iscsi_direct_la_LIBADD = \ + libvirt.la \ + ../gnulib/lib/libgnu.la \ + $(NULL) +endif WITH_STORAGE_ISCSI_DIRECT + if WITH_STORAGE_SCSI libvirt_storage_backend_scsi_la_SOURCES = $(STORAGE_DRIVER_SCSI_SOURCES) libvirt_storage_backend_scsi_la_CFLAGS = \
[...]
diff --git a/src/storage/storage_backend_iscsi_direct.c b/src/storage/storage_backend_iscsi_direct.c new file mode 100644 index 0000000000..e3c1f75b42 --- /dev/null +++ b/src/storage/storage_backend_iscsi_direct.c @@ -0,0 +1,43 @@
Missing copyright comment.
+#include <config.h>
config.h is not a system include so we usually separate it by empty line
+#include <fcntl.h> +#include <sys/stat.h> +#include <sys/wait.h> +#include <unistd.h>
All of these can be removed, they are not needed now.
+ +#include "datatypes.h" +#include "driver.h" +#include "storage_backend_iscsi_direct.h" +#include "storage_util.h" +#include "virlog.h" +#include "virobject.h"
"datatypes.h", "driver.h" and "virobject.h" can be removed from this patche, they are not needed now.
+ +#define VIR_FROM_THIS VIR_FROM_STORAGE + +VIR_LOG_INIT("storage.storage_backend_iscsi_direct"); + + +static int +virStorageBackendISCSIDirectCheckPool(virStoragePoolObjPtr pool ATTRIBUTE_UNUSED, + bool *isActive ATTRIBUTE_UNUSED) +{ + return 0; +} + +static int +virStorageBackendISCSIDirectRefreshPool(virStoragePoolObjPtr pool ATTRIBUTE_UNUSED) +{ + return 0; +} + +virStorageBackend virStorageBackendISCSIDirect = { + .type = VIR_STORAGE_POOL_ISCSI_DIRECT, + + .checkPool = virStorageBackendISCSIDirectCheckPool, + .refreshPool = virStorageBackendISCSIDirectRefreshPool, +}; + +int +virStorageBackendISCSIDirectRegister(void) +{ + return virStorageBackendRegister(&virStorageBackendISCSIDirect); +}
Some minor issues, otherwise looks good. Pavel