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

[Pavel Hrdina]

On Sat, Jul 14, 2018 at 12:06:14AM +0200, c...@lse.epita.fr wrote:
> From: Clementine Hayat 

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 
> ---
>  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, "\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,

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

[clem]

From: Clementine Hayat 

Signed-off-by: Clementine Hayat 
---
 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/configure.ac b/configure.ac
index c668630a79..87ac4dc2c3 100644
--- a/configure.ac
+++ b/configure.ac
@@ -564,6 +564,7 @@ LIBVIRT_STORAGE_ARG_DIR
 LIBVIRT_STORAGE_ARG_FS
 LIBVIRT_STORAGE_ARG_LVM
 LIBVIRT_STORAGE_ARG_ISCSI
+LIBVIRT_STORAGE_ARG_ISCSI_DIRECT
 LIBVIRT_STORAGE_ARG_SCSI
 LIBVIRT_STORAGE_ARG_MPATH
 LIBVIRT_STORAGE_ARG_DISK
@@ -578,6 +579,7 @@ if test "$with_libvirtd" = "no"; then
   with_storage_fs=no
   with_storage_lvm=no
   with_storage_iscsi=no
+  with_storage_iscsi_direct=no
   with_storage_scsi=no
   with_storage_mpath=no
   with_storage_disk=no
@@ -598,6 +600,7 @@ LIBVIRT_STORAGE_CHECK_DIR
 LIBVIRT_STORAGE_CHECK_FS
 LIBVIRT_STORAGE_CHECK_LVM
 LIBVIRT_STORAGE_CHECK_ISCSI
+LIBVIRT_STORAGE_CHECK_ISCSI_DIRECT
 LIBVIRT_STORAGE_CHECK_SCSI
 LIBVIRT_STORAGE_CHECK_MPATH
 LIBVIRT_STORAGE_CHECK_DISK
@@ -608,7 +611,7 @@ LIBVIRT_STORAGE_CHECK_ZFS
 LIBVIRT_STORAGE_CHECK_VSTORAGE
 
 with_storage=no
-for backend in dir fs lvm iscsi scsi mpath rbd disk; do
+for backend in dir fs lvm iscsi iscsi_direct scsi mpath rbd disk; do
 if eval test \$with_storage_$backend = yes; then
 with_storage=yes
 break
@@ -936,6 +939,7 @@ LIBVIRT_STORAGE_RESULT_DIR
 LIBVIRT_STORAGE_RESULT_FS
 LIBVIRT_STORAGE_RESULT_LVM
 LIBVIRT_STORAGE_RESULT_ISCSI
+LIBVIRT_STORAGE_RESULT_ISCSI_DIRECT
 LIBVIRT_STORAGE_RESULT_SCSI
 LIBVIRT_STORAGE_RESULT_MPATH
 LIBVIRT_STORAGE_RESULT_DISK
diff --git a/m4/virt-storage-iscsi-direct.m4 b/m4/virt-storage-iscsi-direct.m4
new file mode 100644
index 00..cc2d490352
--- /dev/null
+++ b/m4/virt-storage-iscsi-direct.m4
@@ -0,0 +1,41 @@
+dnl Iscsi-direct storage
+dnl
+dnl Copyright (C) 2018 Clementine Hayat.
+dnl
+dnl This library is free software; you can redistribute it and/or
+dnl modify it under the terms of the GNU Lesser General Public
+dnl License as published by the Free Software Foundation; either
+dnl version 2.1 of the License, or (at your option) any later version.
+dnl
+dnl This library is distributed in the hope that it will be useful,
+dnl but WITHOUT ANY WARRANTY; without even the implied warranty of
+dnl MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+dnl Lesser General Public License for more details.
+dnl
+dnl You should have received a copy of the GNU Lesser General Public
+dnl License along with this library.  If not, see
+dnl .
+dnl
+
+AC_DEFUN([LIBVIRT_STORAGE_ARG_ISCSI_DIRECT], [
+  LIBVIRT_ARG_WITH_FEATURE([STORAGE_ISCSI_DIRECT],
+   [iscsi-direct backend for the storage driver],
+   [check])
+])
+
+AC_DEFUN([LIBVIRT_STORAGE_CHECK_ISCSI_DIRECT], [
+  AC_REQUIRE([LIBVIRT_CHECK_LIBISCSI])
+  if test "$with_storage_iscsi_direct" = "check"; then
+with_storage_iscsi_direct=$with_libiscsi
+  fi
+  if test "$with_storage_iscsi_direct" = "yes"; then
+AC_DEFINE_UNQUOTED([WITH_STORAGE_ISCSI_DIRECT], [1],
+   [whether iSCSI backend for storage driver is enabled])
+  fi
+  AM_CONDITIONAL([WITH_STORAGE_ISCSI_DIRECT],
+ [test "$with_storage_iscsi_direct" = "yes"])
+])
+
+AC_DEFUN([LIBVIRT_STORAGE_RESULT_ISCSI_DIRECT], [
+  LIBVIRT_RESULT([iscsi-direct], [$with_storage_iscsi_direct])
+])
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",
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",