Re: [PATCH 12/12] qemu: block: add iSER support in qemu block type

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



This is not a properly formatted patch. Please use git send-email for
submissions. Also read https://libvirt.org/hacking.html for coding style
guidelines ...


On Tue, Dec 12, 2017 at 16:53:48 +0800, zhangshengyu@xxxxxxxxxxxxxx wrote:
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 66e21c4bd..c83ce5718 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -4554,6 +4554,11 @@ virDomainDeviceDefPostParseCommon(virDomainDeviceDefPtr dev,
>              disk->src->protocol == VIR_STORAGE_NET_PROTOCOL_ISCSI &&
>              virDomainPostParseCheckISCSIPath(&disk->src->path) < 0)
>              return -1;
> + 
> + if (disk->src->type == VIR_STORAGE_TYPE_NETWORK &&
> +            disk->src->protocol == VIR_STORAGE_NET_PROTOCOL_ISER &&
> +            virDomainPostParseCheckISCSIPath(&disk->src->path) < 0)
> +            return -1;

... like how to format the code properly.

>  
>          if (disk->bus != VIR_DOMAIN_DISK_BUS_VIRTIO &&
>              virDomainCheckVirtioOptions(disk->virtio) < 0)
> @@ -5160,7 +5165,8 @@ virDomainDiskDefValidate(const virDomainDiskDef *disk)
>          if (!(disk->src->type == VIR_STORAGE_TYPE_BLOCK ||
>                disk->src->type == VIR_STORAGE_TYPE_VOLUME ||
>                (disk->src->type == VIR_STORAGE_TYPE_NETWORK &&
> -               disk->src->protocol == VIR_STORAGE_NET_PROTOCOL_ISCSI))) {
> +               (disk->src->protocol == VIR_STORAGE_NET_PROTOCOL_ISCSI ||
> +         disk->src->protocol == VIR_STORAGE_NET_PROTOCOL_ISER)))) {
>              virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
>                             _("disk '%s' improperly configured for a "
>                               "device='lun'"), disk->dst);
> diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c
> index 585f0255e..a9543293d 100644
> --- a/src/qemu/qemu_block.c
> +++ b/src/qemu/qemu_block.c
> @@ -831,7 +831,7 @@ qemuBlockStorageSourceGetISCSIProps(virStorageSourcePtr src)
>                                            "s:portal", portal,
>                                            "s:target", target,
>                                            "u:lun", lun,
> -                                          "s:transport", "tcp",
> +                                          "s:transport", src->protocol == VIR_STORAGE_NET_PROTOCOL_ISER? "iser":"tcp",

too long line.

>                                            "S:user", username,
>                                            "S:password-secret", objalias,
>                                            NULL));
> @@ -1030,6 +1030,7 @@ qemuBlockStorageSourceGetBackendProps(virStorageSourcePtr src)
>              break;
>  
>          case VIR_STORAGE_NET_PROTOCOL_ISCSI:
> +        case VIR_STORAGE_NET_PROTOCOL_ISER:

Is it really a new protocol? Isn't it just a variation of ISCSI. Qemu
treats it as a variation.

>              if (!(fileprops = qemuBlockStorageSourceGetISCSIProps(src)))
>                  return NULL;
>              break;


> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index d7150cae1..76b2a6cc9 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -8133,7 +8133,7 @@ int
>  qemuDomainDefValidateDiskLunSource(const virStorageSource *src)
>  {
>      if (virStorageSourceGetActualType(src) == VIR_STORAGE_TYPE_NETWORK) {
> -        if (src->protocol != VIR_STORAGE_NET_PROTOCOL_ISCSI) {
> +        if (src->protocol != VIR_STORAGE_NET_PROTOCOL_ISCSI && src->protocol != VIR_STORAGE_NET_PROTOCOL_ISER) {
>              virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
>                             _("disk device='lun' is not supported "
>                               "for protocol='%s'"),

[...]

> diff --git a/src/qemu/qemu_parse_command.c b/src/qemu/qemu_parse_command.c
> index 5fe3f97d0..72e137ce4 100644
> --- a/src/qemu/qemu_parse_command.c
> +++ b/src/qemu/qemu_parse_command.c
> @@ -709,6 +709,12 @@ qemuParseCommandLineDisk(virDomainXMLOptionPtr xmlopt,
>  
>                      if (qemuParseISCSIString(def) < 0)
>                          goto error;
> + } else if (STRPREFIX(def->src->path, "iser:")) {
> +                    def->src->type = VIR_STORAGE_TYPE_NETWORK;
> +                    def->src->protocol = VIR_STORAGE_NET_PROTOCOL_ISER;
> +
> +                    if (qemuParseISCSIString(def) < 0)
> +                        goto error;
>                  } else if (STRPREFIX(def->src->path, "sheepdog:")) {
>                      char *p = def->src->path;
>                      char *port, *vdi;
> @@ -2157,6 +2163,7 @@ qemuParseCommandLine(virCapsPtr caps,
>                          goto error;
>  
>                      break;
> +                case VIR_STORAGE_NET_PROTOCOL_ISER:
>                  case VIR_STORAGE_NET_PROTOCOL_ISCSI:
>                      if (qemuParseISCSIString(disk) < 0)
>                          goto error;
> diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
> index 6594715e5..cf5c2904e 100644
> --- a/src/util/virstoragefile.c
> +++ b/src/util/virstoragefile.c

[...]

> @@ -3295,6 +3368,7 @@ static const struct virStorageSourceJSONDriverParser jsonParsers[] = {
>      {"tftp", virStorageSourceParseBackingJSONUri, VIR_STORAGE_NET_PROTOCOL_TFTP},
>      {"gluster", virStorageSourceParseBackingJSONGluster, 0},
>      {"iscsi", virStorageSourceParseBackingJSONiSCSI, 0},
> +    {"iser", virStorageSourceParseBackingJSONiSER, 0},


So this is plain wrong. In qemu it's stil protocol called 'iscsi' with
a transport of 'iser', not a completely new protocol.

citation from qemu.git/qmp/block-core.json:

##
# @IscsiTransport:
#
# An enumeration of libiscsi transport types
#
# Since: 2.9
##
{ 'enum': 'IscsiTransport',
  'data': [ 'tcp', 'iser' ] }

##
# @IscsiHeaderDigest:
#
# An enumeration of header digests supported by libiscsi
#
# Since: 2.9
##
{ 'enum': 'IscsiHeaderDigest',
  'prefix': 'QAPI_ISCSI_HEADER_DIGEST',
  'data': [ 'crc32c', 'none', 'crc32c-none', 'none-crc32c' ] }

##
# @BlockdevOptionsIscsi:
#
# @transport:       The iscsi transport type
#
# @portal:          The address of the iscsi portal
#
# @target:          The target iqn name
#
# @lun:             LUN to connect to. Defaults to 0.
#
# @user:            User name to log in with. If omitted, no CHAP
#                   authentication is performed.
#
# @password-secret: The ID of a QCryptoSecret object providing
#                   the password for the login. This option is required if
#                   @user is specified.
#
# @initiator-name:  The iqn name we want to identify to the target
#                   as. If this option is not specified, an initiator name is
#                   generated automatically.
#
# @header-digest:   The desired header digest. Defaults to
#                   none-crc32c.
#
# @timeout:         Timeout in seconds after which a request will
#                   timeout. 0 means no timeout and is the default.
#
# Driver specific block device options for iscsi
#
# Since: 2.9
##
{ 'struct': 'BlockdevOptionsIscsi',
  'data': { 'transport': 'IscsiTransport',
            'portal': 'str',
            'target': 'str',
            '*lun': 'int',
            '*user': 'str',
            '*password-secret': 'str',
            '*initiator-name': 'str',
            '*header-digest': 'IscsiHeaderDigest',
            '*timeout': 'int' } }


Also it's missing a test case.

>      {"nbd", virStorageSourceParseBackingJSONNbd, 0},
>      {"sheepdog", virStorageSourceParseBackingJSONSheepdog, 0},
>      {"ssh", virStorageSourceParseBackingJSONSSH, 0},

I don't object to adding iser transport per-se, but this patch is a
mess. It's missing proper commit message and the protocol is not a new
protocol but rather a different transport for ISCSI. Please follow the
contributor guidelines before submitting a new patch.

Also please add a case to the qemuxml2argv test so that this addition is
tested properly as I suspect that iser would work only if you enable
authentication in current libvirt since you did not tweak
qemuDiskSourceNeedsProps

Peter

Attachment: signature.asc
Description: PGP signature

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list

[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]
  Powered by Linux