2:50 a.m.
Thanks for all the reviews! I will work on each item and get back.
On Tue, Aug 29, 2017 at 4:00 PM, John Ferlan <jferlan(a)redhat.com> wrote:
Probably need to beef this up a little bit... I can figure something out.
On 08/29/2017 02:39 AM, Ashish Mittal wrote:
> Sample XML for a VxHS disk:
>
> <disk type='network' device='disk'>
> <driver name='qemu' type='raw' cache='none'/>
> <source protocol='vxhs'
name='eb90327c-8302-4725-9e1b-4e85ed4dc251'>
> <host name='192.168.0.1' port='9999'/>
> </source>
> <backingStore/>
> <target dev='vda' bus='virtio'/>
> <serial>eb90327c-8302-4725-9e1b-4e85ed4dc251</serial>
> <alias name='virtio-disk0'/>
> <address type='pci' domain='0x0000' bus='0x00'
slot='0x04' function='0x0'/>
> </disk>
>
> Signed-off-by: Ashish Mittal <Ashish.Mittal(a)veritas.com>
> ---
> v5 changelog:
> (1) Rebased to latest master.
> (2) Review comments from Peter Krempa on patch v4 1/3 have been addressed
>
Not all were, but it's also not something simple to figure out.
> v4 changelog:
> (1) Fixes per review comments from v3.
> (2) Had to remove a test from the previous version that checked for
> error when multiple hosts are specified for VxHS device.
> This started failing virschematest with the error
> "XML document failed to validate against schema" as the
> docs/schemas/domain.rng specifies only a single host.
>
> v3 changelog:
> (1) Implemented the modern syntax for VxHS disk specification.
> (2) Changed qemuxml2argvdata VxHS test case to verify the new syntax.
> (3) Added a negative test case to check failure when multiple hosts
> are specified for a VxHS disk.
>
> v2 changelog:
> (1) Added code for JSON parsing of a VxHS vdisk.
> (2) Added test case to verify JSON parsing.
> (3) Added missing switch-case checks for VIR_STORAGE_NET_PROTOCOL_VXHS.
> (4) Fixed line wrap in qemuxml2argv-disk-drive-network-vxhs.args.
>
> docs/formatdomain.html.in | 15 ++++++++---
> docs/schemas/domaincommon.rng | 13 ++++++++++
> src/libxl/libxl_conf.c | 1 +
> src/qemu/qemu_block.c | 60 +++++++++++++++++++++++++++++++++++++++++++
> src/qemu/qemu_command.c | 9 +++++++
> src/qemu/qemu_driver.c | 3 +++
> src/qemu/qemu_parse_command.c | 15 +++++++++++
> src/util/virstoragefile.c | 40 ++++++++++++++++++++++++++++-
> src/util/virstoragefile.h | 1 +
> src/xenconfig/xen_xl.c | 1 +
> 10 files changed, 154 insertions(+), 4 deletions(-)
>
Which email address would be "preferred" once this gets finalized - work
or gmail? I don't care either way - just need to know as that becomes
part of the git history.
Work email please. Thanks!
The tree is currently "frozen" for the 3.7.0 release, but I
think we can
at least start adding some adjustments for at least the first few
patches once it thaws for 3.8.0. Once the TLS patches start - some more
adjustment will be necessary I think - adjustments that you may need to
make... I can make changes for the first patches as it's mostly simple
things except for the process of capabilities creation...
FWIW: To reduce the change in this one patch - I took the liberty of
creating a patch that will be inserted before this one that only creates
the new symbol and adjusts all the switches appropriately and making you
the author.
You are missing a patch to set up the capabilities - from Peter's review
of v4 patch 1/3
(https://www.redhat.com/archives/libvir-list/2017-June/msg01389.html)
"Additionally since libvirt supports QAPI introspection, this means we
are now able to detect whether qemu supports vxhs and should report an
error if it doesn't.
You'll need to add a capability bit in qemu_capabilities.h and the
detection string in qemu_capabilities.c to virQEMUCapsQMPSchemaQueries[]."
Because libvirt could be run on some host that's not running a QEMU with
the VxHS code we have to have a mechanism that does the appropriate
checks to ensure the underlying QEMU supports what we're trying to a
domain about to be run and generate an appropriate message if it's not
present.
In any case, all that noted, it seems that this can be accomplished by
adding the following line to virQEMUCapsQMPSchemaQueries[]:
"blockdev-add/arg-type/+vxhs"
But there will also need to be a patch to add 2.10 replies and xml data.
Both of these I can do as it's not necessarily intuitively obvious...
So... What I'll do is make some adjustments to this series, then repost
as a v6 so that you (and others) can look.
Thanks! Appreciate the help!
> diff --git a/docs/formatdomain.html.in
b/docs/formatdomain.html.in
> index fba8cfc..64397d4 100644
> --- a/docs/formatdomain.html.in
> +++ b/docs/formatdomain.html.in
> @@ -2520,9 +2520,9 @@
> <dd>
> The <code>protocol</code> attribute specifies the protocol
to
> access to the requested image. Possible values are "nbd",
> - "iscsi", "rbd", "sheepdog" or
"gluster". If the
> - <code>protocol</code> attribute is "rbd",
"sheepdog" or
> - "gluster", an additional attribute
<code>name</code> is
> + "iscsi", "rbd", "sheepdog",
"gluster" or "vxhs". If the
> + <code>protocol</code> attribute is "rbd",
"sheepdog", "gluster"
> + or "vxhs", an additional attribute
<code>name</code> is
> mandatory to specify which volume/image will be used. For
"nbd",
> the <code>name</code> attribute is optional. For
"iscsi"
> (<span class="since">since 1.0.4</span>), the
<code>name</code>
> @@ -2530,6 +2530,9 @@
> target's name by a slash (e.g.,
> <code>iqn.2013-07.com.example:iscsi-pool/1</code>). If
not
> specified, the default LUN is zero.
> + For "vxhs" (<span class="since">since
3.8.0</span>), the
> + <code>name</code> is the UUID of the volume, assigned by
the
> + HyperScale sever.
> <span class="since">Since 0.8.7</span>
> </dd>
> <dt><code>volume</code></dt>
> @@ -2632,6 +2635,12 @@
> <td> one or more (<span class="since">Since
2.1.0</span>), just one prior to that </td>
> <td> 24007 </td>
> </tr>
> + <tr>
> + <td> vxhs </td>
> + <td> a server running Veritas HyperScale daemon </td>
> + <td> only one </td>
> + <td> 9999 </td>
> + </tr>
> </table>
> <p>
> gluster supports "tcp", "rdma", "unix" as
valid values for the
> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
> index 3f56d8f..458b8d8 100644
> --- a/docs/schemas/domaincommon.rng
> +++ b/docs/schemas/domaincommon.rng
> @@ -1642,6 +1642,18 @@
> </element>
> </define>
>
> + <define name="diskSourceNetworkProtocolVxHS">
> + <element name="source">
> + <attribute name="protocol">
> + <choice>
> + <value>vxhs</value>
> + </choice>
> + </attribute>
> + <attribute name="name"/>
> + <ref name="diskSourceNetworkHost"/>
> + </element>
> + </define>
> +
> <define name="diskSourceNetwork">
> <attribute name="type">
> <value>network</value>
> @@ -1652,6 +1664,7 @@
> <ref name="diskSourceNetworkProtocolRBD"/>
> <ref name="diskSourceNetworkProtocolHTTP"/>
> <ref name="diskSourceNetworkProtocolSimple"/>
> + <ref name="diskSourceNetworkProtocolVxHS"/>
> </choice>
> </define>
>
> diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
> index 4416a09..34233a9 100644
> --- a/src/libxl/libxl_conf.c
> +++ b/src/libxl/libxl_conf.c
> @@ -666,6 +666,7 @@ libxlMakeNetworkDiskSrcStr(virStorageSourcePtr src,
> case VIR_STORAGE_NET_PROTOCOL_GLUSTER:
> case VIR_STORAGE_NET_PROTOCOL_SHEEPDOG:
> case VIR_STORAGE_NET_PROTOCOL_SSH:
> + case VIR_STORAGE_NET_PROTOCOL_VXHS:
> case VIR_STORAGE_NET_PROTOCOL_LAST:
> case VIR_STORAGE_NET_PROTOCOL_NONE:
> virReportError(VIR_ERR_NO_SUPPORT,
> diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c
> index 7fb12ea..a4d0160 100644
> --- a/src/qemu/qemu_block.c
> +++ b/src/qemu/qemu_block.c
> @@ -23,6 +23,7 @@
>
> #include "viralloc.h"
> #include "virstring.h"
> +#include "qemu_alias.h"
This isn't needed yet, so I'll remove it now.
>
> #define VIR_FROM_THIS VIR_FROM_QEMU
>
> @@ -482,6 +483,60 @@ qemuBlockStorageSourceGetGlusterProps(virStorageSourcePtr src)
> }
>
>
> +static virJSONValuePtr
> +qemuBuildVxHSDriveJSONHost(virStorageSourcePtr src)
> +{
> + virJSONValuePtr server = NULL;
> + virStorageNetHostDefPtr host;
> + unsigned int port;
> +
> + if (src->nhosts != 1) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("protocol VxHS accepts only one host"));
Since other places use the string "VxHS protocol" - I'll swap these.
> + return NULL;
> + }
> +
> + host = src->hosts;
> + port = host->port;
> +
> + if (virJSONValueObjectCreate(&server,
> + "s:host", host->name,
> + "u:port", port,
> + NULL) < 0)
> + server = NULL;
> +
> + return server;
> +}
> +
> +
> +static virJSONValuePtr
> +qemuBlockStorageSourceGetVxHSProps(virStorageSourcePtr src)
> +{
> + const char *protocol = virStorageNetProtocolTypeToString(src->protocol);
> + virJSONValuePtr server = NULL;
> + virJSONValuePtr ret = NULL;
> +
> + if (!(server = qemuBuildVxHSDriveJSONHost(src)))
> + return NULL;
I see you took the advice from a previous review and created a helper;
however, qemuBlockStorageSourceBuildHostsJSONSocketAddress should
suffice your needs as long as you move the "if (src->nhosts != 1) {"
check moves into here.
It was created from qemuBuildGlusterDriveJSONHosts as part of commit id
'7ee3df577'.
Of course converting to it means adding the ".0" to the .args output.
IIRC, this is something talked about in previous reviews, but now that
we have a general purpose function - we may as well use it.
It will also create a "server.0.type = tcp" entry which probably
wouldn't be a bad thing in this case.
> +
> + /* VxHS disk specification example:
> + * { driver:"vxhs",
> + * vdisk-id:"eb90327c-8302-4725-4e85ed4dc251",
> + * server.host:"1.2.3.4",
> + * server.port:1234}
I'll modify the above to follow the gluster example and be:
server:[{type:"tcp", host:"1.2.3.4", port:9999}]}
NB: 9999 is just a type-a consistency thing
> + */
> + if (virJSONValueObjectCreate(&ret,
> + "s:driver", protocol,
> + "s:vdisk-id", src->path,
> + "a:server", server, NULL) < 0) {
> + virJSONValueFree(server);
> + ret = NULL;
> + }
> +
> + return ret;
> +}
> +
> +
> /**
> * qemuBlockStorageSourceGetBackendProps:
> * @src: disk source
> @@ -512,6 +567,11 @@ qemuBlockStorageSourceGetBackendProps(virStorageSourcePtr src)
> goto cleanup;
> break;
>
> + case VIR_STORAGE_NET_PROTOCOL_VXHS:
> + if (!(fileprops = qemuBlockStorageSourceGetVxHSProps(src)))
> + goto cleanup;
> + break;
> +
> case VIR_STORAGE_NET_PROTOCOL_NBD:
> case VIR_STORAGE_NET_PROTOCOL_RBD:
> case VIR_STORAGE_NET_PROTOCOL_SHEEPDOG:
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index a68ff71..0fd2674 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -991,6 +991,11 @@ qemuBuildNetworkDriveStr(virStorageSourcePtr src,
> ret = virBufferContentAndReset(&buf);
> break;
>
> + case VIR_STORAGE_NET_PROTOCOL_VXHS:
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("'VxHS' protocol does not support URI
syntax"));
I'll drop the single quotes... the 'ssh' is made to look as if someone
called virStorageNetProtocolTypeToString to convert src->protocol to a
string as was done for VIR_STORAGE_NET_PROTOCOL_NBD at the top of this
switch statement.
> + goto cleanup;
> +
> case VIR_STORAGE_NET_PROTOCOL_SSH:
> virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> _("'ssh' protocol is not yet
supported"));
> @@ -1325,6 +1330,10 @@ qemuDiskSourceNeedsProps(virStorageSourcePtr src)
> src->nhosts > 1)
> return true;
>
> + if (actualType == VIR_STORAGE_TYPE_NETWORK &&
> + src->protocol == VIR_STORAGE_NET_PROTOCOL_VXHS)
> + return true;
> +
> return false;
> }
>
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 2ba6c80..da28c4f 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -13732,6 +13732,7 @@
qemuDomainSnapshotPrepareDiskExternalBackingInactive(virDomainDiskDefPtr disk)
> case VIR_STORAGE_NET_PROTOCOL_FTPS:
> case VIR_STORAGE_NET_PROTOCOL_TFTP:
> case VIR_STORAGE_NET_PROTOCOL_SSH:
> + case VIR_STORAGE_NET_PROTOCOL_VXHS:
> case VIR_STORAGE_NET_PROTOCOL_LAST:
> virReportError(VIR_ERR_INTERNAL_ERROR,
> _("external inactive snapshots are not supported on
"
> @@ -13795,6 +13796,7 @@
qemuDomainSnapshotPrepareDiskExternalOverlayActive(virDomainSnapshotDiskDefPtr d
> case VIR_STORAGE_NET_PROTOCOL_FTPS:
> case VIR_STORAGE_NET_PROTOCOL_TFTP:
> case VIR_STORAGE_NET_PROTOCOL_SSH:
> + case VIR_STORAGE_NET_PROTOCOL_VXHS:
> case VIR_STORAGE_NET_PROTOCOL_LAST:
> virReportError(VIR_ERR_INTERNAL_ERROR,
> _("external active snapshots are not supported on
"
> @@ -13940,6 +13942,7 @@ qemuDomainSnapshotPrepareDiskInternal(virConnectPtr conn,
> case VIR_STORAGE_NET_PROTOCOL_FTPS:
> case VIR_STORAGE_NET_PROTOCOL_TFTP:
> case VIR_STORAGE_NET_PROTOCOL_SSH:
> + case VIR_STORAGE_NET_PROTOCOL_VXHS:
> case VIR_STORAGE_NET_PROTOCOL_LAST:
> virReportError(VIR_ERR_INTERNAL_ERROR,
> _("internal inactive snapshots are not supported on
"
> diff --git a/src/qemu/qemu_parse_command.c b/src/qemu/qemu_parse_command.c
> index 8cb96a2..6286c2e 100644
> --- a/src/qemu/qemu_parse_command.c
> +++ b/src/qemu/qemu_parse_command.c
> @@ -736,6 +736,11 @@ qemuParseCommandLineDisk(virDomainXMLOptionPtr xmlopt,
> if (VIR_STRDUP(def->src->path, vdi) < 0)
> goto error;
> }
> + } else if (STRPREFIX(def->src->path, "vxhs:")) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("VxHS protocol does not support URI syntax
'%s'"),
> + def->src->path);
> + goto error;
> } else {
> def->src->type = VIR_STORAGE_TYPE_FILE;
> }
> @@ -1944,6 +1949,10 @@ qemuParseCommandLine(virCapsPtr caps,
> disk->src->type = VIR_STORAGE_TYPE_NETWORK;
> disk->src->protocol = VIR_STORAGE_NET_PROTOCOL_SHEEPDOG;
> val += strlen("sheepdog:");
> + } else if (STRPREFIX(val, "vxhs:")) {
> + disk->src->type = VIR_STORAGE_TYPE_NETWORK;
> + disk->src->protocol = VIR_STORAGE_NET_PROTOCOL_VXHS;
> + val += strlen("vxhs:");
> } else {
> disk->src->type = VIR_STORAGE_TYPE_FILE;
> }
> @@ -2020,6 +2029,12 @@ qemuParseCommandLine(virCapsPtr caps,
> goto error;
>
> break;
> + case VIR_STORAGE_NET_PROTOCOL_VXHS:
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("VxHS protocol does not support URI
"
> + "syntax '%s'"),
disk->src->path);
> + goto error;
> + break;
The break; would never be reached and "theoretically speaking" we should
never get here anyway since it's not possible to use the "older" format.
Still at least you modified qemu_parse_command - not many do...
The rest was fine...
John
Regards,
Ashish
> case VIR_STORAGE_NET_PROTOCOL_HTTP:
> case VIR_STORAGE_NET_PROTOCOL_HTTPS:
> case VIR_STORAGE_NET_PROTOCOL_FTP:
> diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
> index fbc8245..e9a59e0 100644
> --- a/src/util/virstoragefile.c
> +++ b/src/util/virstoragefile.c
> @@ -85,7 +85,8 @@ VIR_ENUM_IMPL(virStorageNetProtocol,
VIR_STORAGE_NET_PROTOCOL_LAST,
> "ftp",
> "ftps",
> "tftp",
> - "ssh")
> + "ssh",
> + "vxhs")
>
> VIR_ENUM_IMPL(virStorageNetHostTransport, VIR_STORAGE_NET_HOST_TRANS_LAST,
> "tcp",
> @@ -2712,6 +2713,7 @@ virStorageSourceParseBackingColon(virStorageSourcePtr src,
> case VIR_STORAGE_NET_PROTOCOL_ISCSI:
> case VIR_STORAGE_NET_PROTOCOL_GLUSTER:
> case VIR_STORAGE_NET_PROTOCOL_SSH:
> + case VIR_STORAGE_NET_PROTOCOL_VXHS:
> virReportError(VIR_ERR_INTERNAL_ERROR,
> _("malformed backing store path for protocol %s"),
> protocol);
> @@ -3210,6 +3212,38 @@ virStorageSourceParseBackingJSONRaw(virStorageSourcePtr src,
> return virStorageSourceParseBackingJSONInternal(src, json);
> }
>
> +static int
> +virStorageSourceParseBackingJSONVxHS(virStorageSourcePtr src,
> + virJSONValuePtr json,
> + int opaque ATTRIBUTE_UNUSED)
> +{
> + const char *vdisk_id = virJSONValueObjectGetString(json, "vdisk-id");
> + virJSONValuePtr server = virJSONValueObjectGetObject(json, "server");
> +
> + if (!vdisk_id || !server) {
> + virReportError(VIR_ERR_INVALID_ARG, "%s",
> + _("missing 'vdisk-id' or 'server'
attribute in "
> + "JSON backing definition for VxHS volume"));
> + return -1;
> + }
> +
> + src->type = VIR_STORAGE_TYPE_NETWORK;
> + src->protocol = VIR_STORAGE_NET_PROTOCOL_VXHS;
> +
> + if (VIR_STRDUP(src->path, vdisk_id) < 0)
> + return -1;
> +
> + if (VIR_ALLOC_N(src->hosts, 1) < 0)
> + return -1;
> + src->nhosts = 1;
> +
> + if (virStorageSourceParseBackingJSONInetSocketAddress(src->hosts,
> + server) < 0)
> + return -1;
> +
> + return 0;
> +}
> +
> struct virStorageSourceJSONDriverParser {
> const char *drvname;
> int (*func)(virStorageSourcePtr src, virJSONValuePtr json, int opaque);
> @@ -3232,6 +3266,7 @@ static const struct virStorageSourceJSONDriverParser
jsonParsers[] = {
> {"ssh", virStorageSourceParseBackingJSONSSH, 0},
> {"rbd", virStorageSourceParseBackingJSONRBD, 0},
> {"raw", virStorageSourceParseBackingJSONRaw, 0},
> + {"vxhs", virStorageSourceParseBackingJSONVxHS, 0},
> };
>
>
> @@ -3992,6 +4027,9 @@ virStorageSourceNetworkDefaultPort(virStorageNetProtocol
protocol)
> /* we don't provide a default for RBD */
> return 0;
>
> + case VIR_STORAGE_NET_PROTOCOL_VXHS:
> + return 9999;
> +
> case VIR_STORAGE_NET_PROTOCOL_LAST:
> case VIR_STORAGE_NET_PROTOCOL_NONE:
> return 0;
> diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h
> index 6c388b1..f7e897f 100644
> --- a/src/util/virstoragefile.h
> +++ b/src/util/virstoragefile.h
> @@ -134,6 +134,7 @@ typedef enum {
> VIR_STORAGE_NET_PROTOCOL_FTPS,
> VIR_STORAGE_NET_PROTOCOL_TFTP,
> VIR_STORAGE_NET_PROTOCOL_SSH,
> + VIR_STORAGE_NET_PROTOCOL_VXHS,
>
> VIR_STORAGE_NET_PROTOCOL_LAST
> } virStorageNetProtocol;
> diff --git a/src/xenconfig/xen_xl.c b/src/xenconfig/xen_xl.c
> index d168d3f..8acbfe3 100644
> --- a/src/xenconfig/xen_xl.c
> +++ b/src/xenconfig/xen_xl.c
> @@ -1024,6 +1024,7 @@ xenFormatXLDiskSrcNet(virStorageSourcePtr src)
> case VIR_STORAGE_NET_PROTOCOL_GLUSTER:
> case VIR_STORAGE_NET_PROTOCOL_SHEEPDOG:
> case VIR_STORAGE_NET_PROTOCOL_SSH:
> + case VIR_STORAGE_NET_PROTOCOL_VXHS:
> case VIR_STORAGE_NET_PROTOCOL_LAST:
> case VIR_STORAGE_NET_PROTOCOL_NONE:
> virReportError(VIR_ERR_NO_SUPPORT,
>