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@xxxxxxxxxx> 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@xxxxxxxxxxx> >> --- >> 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, >> -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
