On Mon, Apr 27, 2020 at 10:01:07 +0800, Han Han wrote:
> The iscsi iser transport is introdcued since QEMU 2.9. For iscsi blockdev
> json, it will be shown at 'transport' field:
> 'json:{...,{"driver": "iscsi","transport":"iser",...}}'
>
> For legacy drive filename as iscsi uri, it will start with 'iser'
> scheme:
> iser://[[username][%<password>]@]<host>[:<port>]/<target-iqn-name>/<lun>
>
> By default, the iscsi transport is still tcp.
>
> Signed-off-by: Han Han <hhan@redhat.com>
> ---
>  src/qemu/qemu_backup.c             |  1 +
>  src/qemu/qemu_block.c              | 16 +++++++++++++---
>  src/qemu/qemu_command.c            | 17 +++++++++++++++++
>  src/qemu/qemu_monitor_json.c       |  1 +
>  src/storage/storage_file_gluster.c |  7 +++++++
>  src/util/virstoragefile.c          | 26 ++++++++++++++++++--------
>  src/util/virstoragefile.h          |  1 +
>  7 files changed, 58 insertions(+), 11 deletions(-)

[...]

> diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c
> index d32277d7..003c18f1 100644
> --- a/src/qemu/qemu_block.c
> +++ b/src/qemu/qemu_block.c
> @@ -418,10 +418,16 @@ qemuBlockStorageSourceGetURI(virStorageSourcePtr src)
>      if (VIR_ALLOC(uri) < 0)
>          return NULL;
> 
> -    if (src->hosts->transport == VIR_STORAGE_NET_HOST_TRANS_TCP) {
> +    if (src->hosts->transport == VIR_STORAGE_NET_HOST_TRANS_TCP ||
> +        src->hosts->transport == VIR_STORAGE_NET_HOST_TRANS_RDMA ||
> +        src->hosts->transport == VIR_STORAGE_NET_HOST_TRANS_ISER)
>          uri->port = src->hosts->port;

RDMA is added here along with ISER. Please add it separately with proper
justification and a test.

Additionally I'm unsure that the design proposed in this patchset is
entirely correct. According to the iSER protocol RFC 5046 iSER is meant

as an extension to the iSCSI protocol. Specifically it's layered on top
of any transport (called RCaP - "RDMA-Capable Protocol" in the RFC).

https://tools.ietf.org/html/rfc5046#section-1.7

Trying to handle it as a transport is thus wrong ...


> +    if (src->hosts->transport == VIR_STORAGE_NET_HOST_TRANS_TCP) {
>          uri->scheme = g_strdup(virStorageNetProtocolTypeToString(src->protocol));
> +    } else if (src->protocol == VIR_STORAGE_NET_PROTOCOL_ISCSI &&
> +               src->hosts->transport == VIR_STORAGE_NET_HOST_TRANS_ISER) {

... and weird. E.g. this code could end-up formatting nbd+iser if the
protocol is not iSCSI in the else branch below.

I'm not even sure whether our use of 'rdma' makes sense, but iSER
appropriated as a transport seems to be even worse.

Unfortunately I don't have any other idea besides changing this to
another network storage protocol rather than transport.

> +        uri->scheme = g_strdup("iser");
>      } else {
>          uri->scheme = g_strdup_printf("%s+%s",
>                                        virStorageNetProtocolTypeToString(src->protocol),
> @@ -497,6 +503,7 @@ qemuBlockStorageSourceBuildJSONSocketAddress(virStorageNetHostDefPtr host,
>              return NULL;
>          break;
> 
> +    case VIR_STORAGE_NET_HOST_TRANS_ISER:
>      case VIR_STORAGE_NET_HOST_TRANS_RDMA:
>      case VIR_STORAGE_NET_HOST_TRANS_LAST:
>          virReportError(VIR_ERR_INTERNAL_ERROR,
> @@ -743,6 +750,7 @@ qemuBlockStorageSourceGetISCSIProps(virStorageSourcePtr src,
>  {
>      qemuDomainStorageSourcePrivatePtr srcPriv = QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(src);
>      g_autofree char *target = NULL;
> +    const char *transport = NULL;
>      char *lunStr = NULL;
>      char *username = NULL;
>      char *objalias = NULL;
> @@ -762,6 +770,7 @@ qemuBlockStorageSourceGetISCSIProps(virStorageSourcePtr src,
>       */
> 
>      target = g_strdup(src->path);
> +    transport = virStorageNetHostTransportTypeToString(src->hosts->transport);
> 
>      /* Separate the target and lun */
>      if ((lunStr = strchr(target, '/'))) {
> @@ -791,7 +800,7 @@ qemuBlockStorageSourceGetISCSIProps(virStorageSourcePtr src,
>                                            "s:portal", portal,
>                                            "s:target", target,
>                                            "u:lun", lun,
> -                                          "s:transport", "tcp",
> +                                          "s:transport", transport,
>                                            "S:user", username,
>                                            "S:password-secret", objalias,
>                                            "S:initiator-name", src->initiator.iqn,
> @@ -2063,7 +2072,8 @@ qemuBlockGetBackingStoreString(virStorageSourcePtr src,
>          /* generate simplified URIs for the easy cases */
>          if (actualType == VIR_STORAGE_TYPE_NETWORK &&
>              src->nhosts == 1 &&
> -            src->hosts->transport == VIR_STORAGE_NET_HOST_TRANS_TCP &&
> +            (src->hosts->transport == VIR_STORAGE_NET_HOST_TRANS_TCP ||
> +             src->hosts->transport == VIR_STORAGE_NET_HOST_TRANS_ISER) &&
>              src->timeout == 0 &&
>              src->ncookies == 0 &&
>              src->sslverify == VIR_TRISTATE_BOOL_ABSENT &&
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 45dd8307..c5cf9401 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -1443,6 +1443,16 @@ qemuCheckDiskConfig(virDomainDiskDefPtr disk,
>                  return -1;
>              }
>          }
> +
> +        if (disk->src &&
> +            disk->src->hosts &&
> +            disk->src->hosts->transport == VIR_STORAGE_NET_HOST_TRANS_ISER &&
> +            !virQEMUCapsGet(qemuCaps, QEMU_CAPS_ISCSI_TRANSPORT_ISER)) {
> +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> +                    _("iSCSI iser transport is not supported by this "
> +                      "QEMU binary"));
> +            return -1;
> +        }
>      }

If the design were okay, this would still require interlocking with
other protocols.

> 
>      if (disk->serial &&
> @@ -4888,6 +4898,13 @@ qemuBuildSCSIiSCSIHostdevDrvStr(virDomainHostdevDefPtr dev,
>      qemuDomainStorageSourcePrivatePtr srcPriv =
>          QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(iscsisrc->src);
> 
> +    if (iscsisrc->src->hosts->transport == VIR_STORAGE_NET_HOST_TRANS_ISER &&
> +        !virQEMUCapsGet(qemuCaps, QEMU_CAPS_ISCSI_TRANSPORT_ISER)) {
> +        virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> +                       _("This QEMU doesn't support iSCSI iser transport"));
> +        return NULL;
> +    }
> +
>      if (qemuDiskSourceNeedsProps(iscsisrc->src, qemuCaps)) {
>          if (!(srcprops = qemuDiskSourceGetProps(iscsisrc->src)))
>              return NULL;

[...]

> diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
> index ffc8bdb3..4f162f10 100644
> --- a/src/util/virstoragefile.c
> +++ b/src/util/virstoragefile.c
> @@ -97,6 +97,7 @@ VIR_ENUM_IMPL(virStorageNetHostTransport,
>                "tcp",
>                "unix",
>                "rdma",
> +              "iser",
>  );
> 
>  VIR_ENUM_IMPL(virStorageSourcePoolMode,
> @@ -2839,10 +2840,15 @@ virStorageSourceParseBackingURI(virStorageSourcePtr src,
> 
>      if (!scheme[0] ||
>          (src->protocol = virStorageNetProtocolTypeFromString(scheme[0])) < 0) {
> -        virReportError(VIR_ERR_INTERNAL_ERROR,
> -                       _("invalid backing protocol '%s'"),
> -                       NULLSTR(scheme[0]));
> -        return -1;
> +        if (STRNEQ(scheme[0], "iser")) {
> +            virReportError(VIR_ERR_INTERNAL_ERROR,
> +                           _("invalid backing protocol '%s'"),
> +                           NULLSTR(scheme[0]));
> +            return -1;
> +        }
> +
> +        src->protocol = VIR_STORAGE_NET_PROTOCOL_ISCSI;
> +        src->hosts->transport = VIR_STORAGE_NET_HOST_TRANS_ISER;
>      }
> 
>      if (scheme[1] &&
> @@ -3523,14 +3529,17 @@ virStorageSourceParseBackingJSONiSCSI(virStorageSourcePtr src,
>          return -1;
> 
>      src->nhosts = 1;
> +    src->hosts->transport = VIR_STORAGE_NET_HOST_TRANS_TCP;
> 
> -    if (STRNEQ_NULLABLE(transport, "tcp")) {
> +    if (STRNEQ(transport, "tcp") && STRNEQ(transport, "iser") && transport) {

If 'transport' is NULL, this will crash on the first strneq. You'd have
to check it first.


>          virReportError(VIR_ERR_INVALID_ARG, "%s",
> -                       _("only TCP transport is supported for iSCSI volumes"));
> +                       _("only TCP or iSER transport is supported for iSCSI "
> +                         "volumes"));
>          return -1;
>      }
> 
> -    src->hosts->transport = VIR_STORAGE_NET_HOST_TRANS_TCP;
> +    if (transport)
> +        src->hosts->transport = virStorageNetHostTransportTypeFromString(transport);
> 
>      if (!portal) {
>          virReportError(VIR_ERR_INVALID_ARG, "%s",
> @@ -4710,7 +4719,8 @@ virStorageSourceNetworkAssignDefaultPorts(virStorageSourcePtr src)
>      size_t i;
> 
>      for (i = 0; i < src->nhosts; i++) {
> -        if (src->hosts[i].transport == VIR_STORAGE_NET_HOST_TRANS_TCP &&
> +        if ((src->hosts[i].transport == VIR_STORAGE_NET_HOST_TRANS_TCP ||
> +             src->hosts[i].transport == VIR_STORAGE_NET_HOST_TRANS_ISER) &&

This would assume that iSER has addressing equal to TCP, but if layered
on top of something else that might not be the case.

>              src->hosts[i].port == 0)
>              src->hosts[i].port = virStorageSourceNetworkDefaultPort(src->protocol);
>      }