On 01/27/2017 08:43 PM, ashish mittal wrote: > Thanks for the review! > > My inputs on some of the comments - > > On Wed, Jan 25, 2017 at 7:59 AM, John Ferlan <jferlan@xxxxxxxxxx> wrote: >> >> >> On 01/19/2017 09:21 PM, Ashish Mittal wrote: >>> Sample XML for a vxhs vdisk is as follows: >>> >>> <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> >> >> It's still not really clear how someone knows to use the name string. >> IOW: How would someone know what to supply. Perhaps the libvirt wiki >> (http://wiki.libvirt.org/page/Main_Page) as opposed to the libvirt docs. >> >> I assume that someone using VxHS knows how to get that, but still I find >> it a "good thing" to be able to have that description somewhere as I can >> only assume some day it'll come up. >> >> But since we don't "require" this for iSCSI, Ceph/RBD, Gluster, etc. >> it's not something 'required' for this patch. Still something for >> someone's todo list to get a description on the libvirt wiki. Once you >> have wiki write access, then you can always keep that data up to date >> without requiring libvirt patches. >> >>> >>> Signed-off-by: Ashish Mittal <Ashish.Mittal@xxxxxxxxxxx> >>> --- >>> 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. >>> >>> 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. >>> >>> docs/formatdomain.html.in | 15 ++++- >>> docs/schemas/domaincommon.rng | 1 + >>> src/libxl/libxl_conf.c | 1 + >>> src/qemu/qemu_command.c | 68 ++++++++++++++++++++++ >>> src/qemu/qemu_driver.c | 3 + >>> src/qemu/qemu_parse_command.c | 26 +++++++++ >>> src/util/virstoragefile.c | 63 +++++++++++++++++++- >>> src/util/virstoragefile.h | 1 + >>> src/xenconfig/xen_xl.c | 1 + >>> ...xml2argv-disk-drive-network-vxhs-multi-host.xml | 35 +++++++++++ >>> .../qemuxml2argv-disk-drive-network-vxhs.args | 25 ++++++++ >>> .../qemuxml2argv-disk-drive-network-vxhs.xml | 34 +++++++++++ >>> tests/qemuxml2argvtest.c | 2 + >>> tests/virstoragetest.c | 16 +++++ >>> 14 files changed, 287 insertions(+), 4 deletions(-) >>> create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-vxhs-multi-host.xml >>> create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-vxhs.args >>> create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-vxhs.xml >>> >> >> In general things are looking much better - a couple of nits below and 2 >> things to consider/rework... >> >> #1. Based on Peter's v2 comments, we don't want to support the >> older/legacy syntax for VxHS, so it's something that should be removed - >> although we should check for it being present and fail if found. >> > > I am testing with changed code to return error if legacy syntax is > found for VxHS. Also added a test case to check for failure on legacy > syntax and it seems to pass (test #41 below). > > Then I added a pass test case to check conversion from new native > syntax to XML (test #40 below). That test fails with error > 'qemuParseCommandLineDisk:901 : internal error: missing file parameter > in drive 'file.driver=vxhs,file.vdisk-id=eb90327c-8302-4725-9e1b...' The qemu_parse_command.c changes while nice to have weren't even updated when multiple gluster servers were added (e.g. commit id '' or '7b7da9e28') Check the changes to add the new s IOW: This code knows how to parse something like: -drive 'file=gluster+unix:///Volume2/Image?socket=/path/to/sock,format=raw,if=none,id=drive-virtio-disk1' but it's clueless for: -drive file.driver=gluster,file.volume=Volume3,file.path=/Image.qcow2,\ file.server.0.type=tcp,file.server.0.host=example.org,file.server.0.port=6000,\ file.server.1.type=tcp,file.server.1.host=example.org,file.server.1.port=24007,\ file.server.2.type=unix,file.server.2.socket=/path/to/sock,format=qcow2,\ if=none,id=drive-virtio-disk2 \ -device virtio-blk-pci,bus=pci.0,addr=0x5,drive=drive-virtio-disk2,\ id=virtio-disk2 See > > Looks like none of the existing tests in qemuargv2xmltest test for the > parsing of new syntax, and qemuParseCommandLineDisk() expects to find > 'file=' for a drive or it errors out. If this is true, will it be able > to parse the new syntax? Some help here please! > > Output from the newly added test cases (40 should pass and 41 checks > for error) : > > 40) QEMU ARGV-2-XML disk-drive-network-vxhs > ... Got unexpected warning from qemuParseCommandLineString: > 2017-01-28 00:57:30.814+0000: 10391: info : libvirt version: 3.0.0 > 2017-01-28 00:57:30.814+0000: 10391: info : hostname: localhost.localdomain > 2017-01-28 00:57:30.814+0000: 10391: error : > qemuParseCommandLineDisk:901 : internal error: missing file parameter > in drive 'file.driver=vxhs,file.vdisk-id=eb90327c-8302-4725-9e1b-4e85ed4dc251,file.server.host=192.168.0.1,file.server.port=9999,format=raw,if=none,id=drive-virtio-disk0,cache=none' > libvirt: QEMU Driver error : internal error: missing file parameter in > drive 'file.driver=vxhs,file.vdisk-id=eb90327c-8302-4725-9e1b-4e85ed4dc251,file.server.host=192.168.0.1,file.server.port=9999,format=raw,if=none,id=drive-virtio-disk0,cache=none' > FAILED > > 41) QEMU ARGV-2-XML disk-drive-network-vxhs-fail > ... Got expected error from qemuParseCommandLineString: > libvirt: QEMU Driver error : internal error: VxHS protocol does not > support URI syntax > 'vxhs://192.168.0.1:9999/eb90327c-8302-4725-9e1b-4e85ed4dc251' > OK > 42) QEMU ARGV-2-XML disk-usb ... OK > > > >> #2. Is the desire to ever support more than 1 host? If not, then is the >> "server" syntax you've borrowed from the Gluster code necessary? Could >> you just go with the single "host" like NBD and SSH. As it relates to >> the qemu command line - I'm not quite as clear. From the example I see >> in commit id '7b7da9e28', the gluster syntax would have: >> > > Present understanding is to have only one host. You are right, the > "server" part is not necessary. Will have to check with the qemu > community on this change. > >> +file.server.0.type=tcp,file.server.0.host=example.org,file.server.0.port=6000,\ >> +file.server.1.type=tcp,file.server.1.host=example.org,file.server.1.port=24007,\ >> +file.server.2.type=unix,file.server.2.socket=/path/to/sock,format=qcow2,\ >> >> whereas, the VxHS syntax is: >> +file.server.host=192.168.0.1,file.server.port=9999,format=raw,if=none,\ >> >> FWIW: I also note there is no ".type=tcp" in your output - so perhaps >> the "default" is tcp unless otherwise specified, but I'm sure of the >> qemu syntax requirements in this area. I assume that since there's only >> 1 server, the ".0, .1, .2" become unnecessary (something added by commit >> id 'f1bbc7df4' for multiple gluster hosts). >> > > That's correct. TCP is the default. > >> I haven't closedly followed the qemu syntax discussion, but it would it >> would be possible to use: >> >> +file.host=192.168.0.1,file.port=9999 >> > > That is correct. Above syntax would also work for us. I will pose this > suggestion to the qemu community and update with their response. > >> Similar to how NBD (see commit id 'a1674fd9') and SSH (see commit id >> 'bc225b1b5') are handled. >> >>> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in >>> index f7bef51..2a071c9 100644 >>> --- a/docs/formatdomain.html.in >>> +++ b/docs/formatdomain.html.in >>> @@ -2319,9 +2319,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> >>> @@ -2329,6 +2329,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 2.5.0</span>), the >> >> Next release is 3.1.0 >> >>> + <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> >>> @@ -2431,6 +2434,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 4d76315..cdc39ca 100644 >>> --- a/docs/schemas/domaincommon.rng >>> +++ b/docs/schemas/domaincommon.rng >>> @@ -1470,6 +1470,7 @@ >>> <value>ftp</value> >>> <value>ftps</value> >>> <value>tftp</value> >>> + <value>vxhs</value> >>> </choice> >>> </attribute> >>> <optional> >>> diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c >>> index b569dda..7e12d32 100644 >>> --- a/src/libxl/libxl_conf.c >>> +++ b/src/libxl/libxl_conf.c >>> @@ -604,6 +604,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_command.c b/src/qemu/qemu_command.c >>> index f8e48d2..08bad8e 100644 >>> --- a/src/qemu/qemu_command.c >>> +++ b/src/qemu/qemu_command.c >>> @@ -491,6 +491,9 @@ qemuNetworkDriveGetPort(int protocol, >>> /* no default port specified */ >>> return 0; >>> >>> + case VIR_STORAGE_NET_PROTOCOL_VXHS: >>> + return 9999; >>> + >>> case VIR_STORAGE_NET_PROTOCOL_RBD: >>> case VIR_STORAGE_NET_PROTOCOL_LAST: >>> case VIR_STORAGE_NET_PROTOCOL_NONE: >>> @@ -899,6 +902,65 @@ qemuBuildGlusterDriveJSON(virStorageSourcePtr src) >>> } >>> >>> >>> +#define QEMU_DEFAULT_VXHS_PORT "9999" >>> + >>> +/* Build the VxHS host object */ >>> +static virJSONValuePtr >>> +qemuBuildVxHSDriveJSONHost(virStorageSourcePtr src) >>> +{ >>> + virJSONValuePtr server = NULL; >>> + virStorageNetHostDefPtr host; >>> + const char *portstr; >>> + >>> + if (src->nhosts != 1) { >>> + virReportError(VIR_ERR_INTERNAL_ERROR, >>> + _("VxHS supports only one server")); >> >> make syntax-check failure here - need a format, e.g. >> >> virReportError(VIR_ERR_INTERNAL_ERROR, "%s", >> >>> + goto cleanup; >>> + } >>> + >>> + host = src->hosts; >>> + portstr = host->port; >>> + >>> + if (!portstr) >>> + portstr = QEMU_DEFAULT_VXHS_PORT; >>> + >>> + if (virJSONValueObjectCreate(&server, >>> + "s:host", host->name, >>> + "s:port", portstr, >>> + NULL) < 0) >>> + server = NULL; >>> + >>> + cleanup: >>> + return server; >>> +} >>> + >>> + >>> +static virJSONValuePtr >>> +qemuBuildVxHSDriveJSON(virStorageSourcePtr src) >>> +{ >>> + const char *protocol = virStorageNetProtocolTypeToString(src->protocol); >>> + virJSONValuePtr server = NULL; >>> + virJSONValuePtr ret = NULL; >>> + >>> + if (!(server = qemuBuildVxHSDriveJSONHost(src))) >>> + return NULL; >>> + >>> + /* VxHS disk sepecification example: >> >> "specification" >> >>> + * { driver:"vxhs", >>> + * vdisk-id:"eb90327c-8302-4725-4e85ed4dc251", >>> + * server.host:"1.2.3.4", >>> + * server.port:1234} >>> + */ >>> + if (virJSONValueObjectCreate(&ret, >>> + "s:driver", protocol, >>> + "s:vdisk-id", src->path, >>> + "a:server", server, NULL) < 0) >> >> If you're going with the 1 host only model, then I believe this isn't >> necessary. For the Gluster code there's a difference based solely on >> whether >1 hosts are present whether the "server:" syntax is added or >> the legacy syntax is used. >> >> If you're not ever going to allow multiple hosts, the "server" option >> would seem to be unnecessary... Rather I would think you should just be >> able to add "s:host" and "s:port" type options. >> >>> + virJSONValueFree(server); >>> + >>> + return ret; >>> +} >>> + >>> + >>> static char * >>> qemuBuildNetworkDriveURI(virStorageSourcePtr src, >>> qemuDomainSecretInfoPtr secinfo) >>> @@ -1034,6 +1096,7 @@ qemuBuildNetworkDriveStr(virStorageSourcePtr src, >>> case VIR_STORAGE_NET_PROTOCOL_TFTP: >>> case VIR_STORAGE_NET_PROTOCOL_ISCSI: >>> case VIR_STORAGE_NET_PROTOCOL_GLUSTER: >>> + case VIR_STORAGE_NET_PROTOCOL_VXHS: >>> ret = qemuBuildNetworkDriveURI(src, secinfo); >>> break; >>> >>> @@ -1148,6 +1211,11 @@ qemuGetDriveSourceProps(virStorageSourcePtr src, >>> if (!(fileprops = qemuBuildGlusterDriveJSON(src))) >>> return -1; >>> } >>> + >>> + if (src->protocol == VIR_STORAGE_NET_PROTOCOL_VXHS) { >>> + if (!(fileprops = qemuBuildVxHSDriveJSON(src))) >>> + return -1; >>> + } >>> break; >>> } >>> >>> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c >>> index b359e77..c76e9d4 100644 >>> --- a/src/qemu/qemu_driver.c >>> +++ b/src/qemu/qemu_driver.c >>> @@ -13788,6 +13788,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 " >>> @@ -13851,6 +13852,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 " >>> @@ -13996,6 +13998,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 405e655..aab287a 100644 >>> --- a/src/qemu/qemu_parse_command.c >>> +++ b/src/qemu/qemu_parse_command.c >>> @@ -263,6 +263,17 @@ qemuParseNBDString(virDomainDiskDefPtr disk) >>> return -1; >>> } >>> >>> +static int >>> +qemuParseVxHSString(virDomainDiskDefPtr def) >>> +{ >>> + virURIPtr uri = NULL; >>> + >>> + if (!(uri = virURIParse(def->src->path))) >>> + return -1; >>> + >>> + return qemuParseDriveURIString(def, uri, "vxhs"); >>> +} >>> + >>> >>> /* >>> * This method takes a string representing a QEMU command line ARGV set >>> @@ -737,6 +748,12 @@ qemuParseCommandLineDisk(virDomainXMLOptionPtr xmlopt, >>> if (VIR_STRDUP(def->src->path, vdi) < 0) >>> goto error; >>> } >>> + } else if (STRPREFIX(def->src->path, "vxhs:")) { >>> + def->src->type = VIR_STORAGE_TYPE_NETWORK; >>> + def->src->protocol = VIR_STORAGE_NET_PROTOCOL_VXHS; >>> + >>> + if (qemuParseVxHSString(def) < 0) >>> + goto error; >>> } else { >>> def->src->type = VIR_STORAGE_TYPE_FILE; >>> } >>> @@ -1947,6 +1964,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; >>> } >>> @@ -2023,6 +2044,11 @@ qemuParseCommandLine(virCapsPtr caps, >>> goto error; >>> >>> break; >>> + case VIR_STORAGE_NET_PROTOCOL_VXHS: >>> + if (qemuParseVxHSString(disk) < 0) >>> + goto error; >>> + >>> + break; >>> 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 ce6d213..e62f4e6 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", >>> @@ -2633,6 +2634,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); >>> @@ -2964,6 +2966,64 @@ virStorageSourceParseBackingJSONRBD(virStorageSourcePtr src, >>> } >>> >>> >>> +static int >>> +virStorageSourceParseBackingJSONVXHS(virStorageSourcePtr src, >> >> s/VXHS/VxHS (to be consistent) >> >>> + virJSONValuePtr json, >>> + int opaque ATTRIBUTE_UNUSED) >> >> These two arguments need to be aligned under the 'v' instead of under >> the '(' >> >>> +{ >>> + virJSONValuePtr server; >>> + const char *hostname, *port; >> >> Single lines for each and follow existing model - e.g. move the lines >> below setting these to here. >> >>> + const char *uri = virJSONValueObjectGetString(json, "filename"); >>> + const char *vdisk_id = virJSONValueObjectGetString(json, "vdisk-id"); >>> + >>> + src->type = VIR_STORAGE_TYPE_NETWORK; >>> + src->protocol = VIR_STORAGE_NET_PROTOCOL_VXHS; >>> + >>> + /* legacy URI based syntax passed via 'filename' option */ >>> + if (uri) >>> + return virStorageSourceParseBackingJSONUriStr(src, uri, >>> + VIR_STORAGE_NET_PROTOCOL_VXHS); >> >> Since we don't want to support the legacy option - this becomes a >> failure path with some sort of error indicating unsupported format. >> >> For the other protocols, the legacy exists because 'filename' existed >> prior to when the "new" syntax support was added so both need to be >> supported. >> >>> + >>> + server = virJSONValueObjectGetObject(json, "server"); >>> + hostname = virJSONValueObjectGetString(server, "host"); >>> + port = virJSONValueObjectGetString(server, "port"); >> >> Each of these should be set above similar to how each of the other >> helpers do this. Although whether "server" is necessary depends on the >> multiple hosts decision. The *JSONGluster uses it because the legacy >> syntax supports only one "host", while the non legacy syntax supports >> having multiple hosts (although it's really not well described in/from >> commit id '7b7da9e2833"). >> >>> + >>> + if (!vdisk_id) { >>> + virReportError(VIR_ERR_INVALID_ARG, "%s", >>> + _("missing 'vdisk-id' attribute in " >>> + "JSON backing definition for VxHS volume")); >>> + return -1; >>> + } >>> + if (!server) { >>> + virReportError(VIR_ERR_INVALID_ARG, "%s", >>> + _("missing 'server' attribute in " >>> + "JSON backing definition for VxHS volume")); >>> + return -1; >>> + } >>> + if (!hostname) { >>> + virReportError(VIR_ERR_INVALID_ARG, "%s", >>> + _("missing hostname for tcp backing server in " >>> + "JSON backing definition for VxHS volume")); >>> + return -1; >>> + } >> >> You could combine these to be like others and just have one error message. >> >>> + >>> + >>> + if (VIR_STRDUP(src->path, vdisk_id) < 0) >>> + return -1; >>> + >>> + if (VIR_ALLOC_N(src->hosts, 1) < 0) >>> + return -1; >>> + src->nhosts = 1; >>> + >>> + src->hosts[0].transport = VIR_STORAGE_NET_HOST_TRANS_TCP; >>> + >>> + if (VIR_STRDUP(src->hosts[0].name, hostname) < 0 || >>> + VIR_STRDUP(src->hosts[0].port, port) < 0) >>> + return -1; >>> + >>> + return 0; >>> +} >>> + >>> struct virStorageSourceJSONDriverParser { >>> const char *drvname; >>> int (*func)(virStorageSourcePtr src, virJSONValuePtr json, int opaque); >>> @@ -2985,6 +3045,7 @@ static const struct virStorageSourceJSONDriverParser jsonParsers[] = { >>> {"sheepdog", virStorageSourceParseBackingJSONSheepdog, 0}, >>> {"ssh", virStorageSourceParseBackingJSONSSH, 0}, >>> {"rbd", virStorageSourceParseBackingJSONRBD, 0}, >>> + {"vxhs", virStorageSourceParseBackingJSONVXHS, 0}, >>> }; >>> >>> >>> diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h >>> index 1f62244..e60b6e9 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 18d9fe3..9fe24d6 100644 >>> --- a/src/xenconfig/xen_xl.c >>> +++ b/src/xenconfig/xen_xl.c >>> @@ -942,6 +942,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, >>> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-vxhs-multi-host.xml b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-vxhs-multi-host.xml >>> new file mode 100644 >>> index 0000000..660bcde >>> --- /dev/null >>> +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-vxhs-multi-host.xml >>> @@ -0,0 +1,35 @@ >>> +<domain type='qemu'> >>> + <name>QEMUGuest1</name> >>> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> >>> + <memory unit='KiB'>219136</memory> >>> + <currentMemory unit='KiB'>219136</currentMemory> >>> + <vcpu placement='static'>1</vcpu> >>> + <os> >>> + <type arch='i686' machine='pc'>hvm</type> >>> + <boot dev='hd'/> >>> + </os> >>> + <clock offset='utc'/> >>> + <on_poweroff>destroy</on_poweroff> >>> + <on_reboot>restart</on_reboot> >>> + <on_crash>destroy</on_crash> >>> + <devices> >>> + <emulator>/usr/libexec/qemu-kvm</emulator> >>> + <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'/> >>> + <host name='192.168.0.2' 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> >>> + <controller type='usb' index='0'/> >>> + <controller type='pci' index='0' model='pci-root'/> >>> + <input type='mouse' bus='ps2'/> >>> + <input type='keyboard' bus='ps2'/> >>> + <memballoon model='none'/> >>> + </devices> >>> +</domain> >>> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-vxhs.args b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-vxhs.args >>> new file mode 100644 >>> index 0000000..23045e1 >>> --- /dev/null >>> +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-vxhs.args >>> @@ -0,0 +1,25 @@ >>> +LC_ALL=C \ >>> +PATH=/bin \ >>> +HOME=/home/test \ >>> +USER=test \ >>> +LOGNAME=test \ >>> +QEMU_AUDIO_DRV=none \ >>> +/usr/libexec/qemu-kvm \ >>> +-name QEMUGuest1 \ >>> +-S \ >>> +-M pc \ >>> +-cpu qemu32 \ >>> +-m 214 \ >>> +-smp 1,sockets=1,cores=1,threads=1 \ >>> +-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \ >>> +-nographic \ >>> +-nodefaults \ >>> +-monitor unix:/tmp/lib/domain--1-QEMUGuest1/monitor.sock,server,nowait \ >>> +-no-acpi \ >>> +-boot c \ >>> +-usb \ >>> +-drive file.driver=vxhs,file.vdisk-id=eb90327c-8302-4725-9e1b-4e85ed4dc251,\ >>> +file.server.host=192.168.0.1,file.server.port=9999,format=raw,if=none,\ >>> +id=drive-virtio-disk0,cache=none \ >>> +-device virtio-blk-pci,bus=pci.0,addr=0x4,drive=drive-virtio-disk0,\ >>> +id=virtio-disk0 >>> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-vxhs.xml b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-vxhs.xml >>> new file mode 100644 >>> index 0000000..45c807f >>> --- /dev/null >>> +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-vxhs.xml >>> @@ -0,0 +1,34 @@ >>> +<domain type='qemu'> >>> + <name>QEMUGuest1</name> >>> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> >>> + <memory unit='KiB'>219136</memory> >>> + <currentMemory unit='KiB'>219136</currentMemory> >>> + <vcpu placement='static'>1</vcpu> >>> + <os> >>> + <type arch='i686' machine='pc'>hvm</type> >>> + <boot dev='hd'/> >>> + </os> >>> + <clock offset='utc'/> >>> + <on_poweroff>destroy</on_poweroff> >>> + <on_reboot>restart</on_reboot> >>> + <on_crash>destroy</on_crash> >>> + <devices> >>> + <emulator>/usr/libexec/qemu-kvm</emulator> >>> + <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> >>> + <controller type='usb' index='0'/> >>> + <controller type='pci' index='0' model='pci-root'/> >>> + <input type='mouse' bus='ps2'/> >>> + <input type='keyboard' bus='ps2'/> >>> + <memballoon model='none'/> >>> + </devices> >>> +</domain> >>> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c >>> index ab3ad08..f751ec9 100644 >>> --- a/tests/qemuxml2argvtest.c >>> +++ b/tests/qemuxml2argvtest.c >>> @@ -892,6 +892,8 @@ mymain(void) >>> # endif >>> DO_TEST("disk-drive-network-rbd-ipv6", NONE); >>> DO_TEST_FAILURE("disk-drive-network-rbd-no-colon", NONE); >>> + DO_TEST("disk-drive-network-vxhs", NONE); >>> + DO_TEST_FAILURE("disk-drive-network-vxhs-multi-host", NONE); >>> DO_TEST("disk-drive-no-boot", >>> QEMU_CAPS_BOOTINDEX); >>> DO_TEST_PARSE_ERROR("disk-device-lun-type-invalid", >>> diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c >>> index f766df1..a28fbd9 100644 >>> --- a/tests/virstoragetest.c >>> +++ b/tests/virstoragetest.c >>> @@ -1492,6 +1492,22 @@ mymain(void) >>> "<source protocol='rbd' name='testshare'>\n" >>> " <host name='example.com'/>\n" >>> "</source>\n"); >>> + TEST_BACKING_PARSE("json:{\"file.driver\":\"vxhs\"," >>> + "\"file.filename\":\"vxhs://192.168.0.1:9999/c6718f6b-0401-441d-a8c3-1f0064d75ee0\"" >>> + "}", >>> + "<source protocol='vxhs' name='c6718f6b-0401-441d-a8c3-1f0064d75ee0'>\n" >>> + " <host name='192.168.0.1' port='9999'/>\n" >>> + "</source>\n"); >>> + TEST_BACKING_PARSE("json:{\"file\":{\"driver\":\"vxhs\"," >>> + "\"vdisk-id\":\"c6718f6b-0401-441d-a8c3-1f0064d75ee0\"," >>> + "\"server\": { \"host\":\"example.com\"," >>> + "\"port\":\"1234\"" >>> + "}" >>> + "}" >>> + "}", >>> + "<source protocol='vxhs' name='c6718f6b-0401-441d-a8c3-1f0064d75ee0'>\n" >>> + " <host name='example.com' port='1234'/>\n" >>> + "</source>\n"); >> >> Since the desire is to not support the older "driver=vxhs,..." syntax >> that means this second example would result in an error. >> >> If you wanted to "check" that it does error, then create a >> TEST_BACKING_PARSE_ERROR macro that *expects* virTestRun to fail and use >> the older syntax with that. >> >> John >> >>> >>> cleanup: >>> /* Final cleanup */ >>> -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
