On Sat, Jul 14, 2018 at 12:06:14AM +0200, clem@xxxxxxxxxxxx wrote:
> From: Clementine Hayat <clem@xxxxxxxxxxxx>
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@xxxxxxxxxxxx>
> ---
> 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
Attachment:
signature.asc
Description: PGP signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
