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@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@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,

On 08/29/2017 02:39 AM, Ashish Mittal wrote:

Signed-off-by: Ashish Mittal <Ashish.Mittal@veritas.com> --- .../qemuxml2argv-disk-drive-network-vxhs.args | 27 +++++++++++++++++ .../qemuxml2argv-disk-drive-network-vxhs.xml | 34 ++++++++++++++++++++++ tests/qemuxml2argvtest.c | 1 + 3 files changed, 62 insertions(+) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-vxhs.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-vxhs.xml

Could have been merged with previous, but as long as there's tests that's good. I'll add a qemuxml2xmltest.c option and a qemuxml2xmlout-disk-drive-network-vxhs.xml file There would be an adjustment to the .args output due the "file.server.0" now instead of just "file.server" and of course the "file.server.0.type = tcp,". I assume that works with the qemu changes anyway.

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..1747dc8 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-vxhs.args @@ -0,0 +1,27 @@ +LC_ALL=C \ +PATH=/bin \ +HOME=/home/test \ +USER=test \ +LOGNAME=test \ +QEMU_AUDIO_DRV=none \ +/usr/bin/qemu-system-x86_64 \ +-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 \ +-chardev socket,id=charmonitor,path=/tmp/lib/domain--1-QEMUGuest1/monitor.sock,\ +server,nowait \ +-mon chardev=charmonitor,id=monitor,mode=readline \ +-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..a488770 --- /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/bin/qemu-system-x86_64</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/>

This is unnecessary, I'll remove (it'll cause xml2xml errors too)

+ <target dev='vda' bus='virtio'/> + <serial>eb90327c-8302-4725-9e1b-4e85ed4dc251</serial> + <alias name='virtio-disk0'/>

Likewise, so I'll remove John

+ <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 5cdbc78..f7dbf39 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -931,6 +931,7 @@ 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("disk-drive-no-boot", QEMU_CAPS_BOOTINDEX); DO_TEST_PARSE_ERROR("disk-device-lun-type-invalid",

On 08/29/2017 02:39 AM, Ashish Mittal wrote:

Signed-off-by: Ashish Mittal <Ashish.Mittal@veritas.com> --- tests/virstoragetest.c | 10 ++++++++++ 1 file changed, 10 insertions(+)

Similarly this probably should be merged with the code that alters qemu_{command|block}...

diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c index d83db78..f8444e1 100644 --- a/tests/virstoragetest.c +++ b/tests/virstoragetest.c @@ -1585,6 +1585,16 @@ mymain(void) "<source protocol='sheepdog' name='Alice'>\n" " <host name='10.10.10.10' port='7000'/>\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");

For consistency, I'll modify port to be 9999 and I'll add the "type:tcp," string as well to match previous change John

#endif /* WITH_YAJL */

cleanup:

On 08/29/2017 02:39 AM, Ashish Mittal wrote:

Add a new TLS X.509 certificate type - "vxhs". This will handle the creation of a TLS certificate capability for properly configured VxHS network block device clients.

Signed-off-by: Ashish Mittal <Ashish.Mittal@veritas.com> --- v5 changelog: (1) Fixed the release version for VxHS changes. (2) No functional changes.

v4 changelog: (1) Add two new options in /etc/libvirt/qemu.conf to control TLS behavior with VxHS block devices "vxhs_tls" and "vxhs_tls_x509_cert_dir". (2) Setting "vxhs_tls=1" in /etc/libvirt/qemu.conf will enable TLS for VxHS block devices. (3) "vxhs_tls_x509_cert_dir" can be set to the full path where the TLS certificates and keys are saved. If this value is missing, the "default_tls_x509_cert_dir" will be used instead.

docs/formatdomain.html.in | 16 ++++++++++++++++ src/qemu/libvirtd_qemu.aug | 4 ++++ src/qemu/qemu.conf | 23 +++++++++++++++++++++++ src/qemu/qemu_conf.c | 7 +++++++ src/qemu/qemu_conf.h | 3 +++ src/qemu/test_libvirtd_qemu.aug.in | 2 ++ 6 files changed, 55 insertions(+)

Starting with this patch - I think there's a bit more work to do... We just need to think logically through things based on the VxHS usage model.

diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 64397d4..41b4ea8 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -2649,6 +2649,22 @@ transport is "unix", the socket attribute specifies the path to an AF_UNIX socket. </p> + <p> + <span class="since">Since 3.8.0,</span> the optional attribute + <code>tls</code> (QEMU only) can be used to control whether a vxhs

s/(QEMU only) // s/vxhs/VxHS/

+ network block device would utilize a hypervisor configured + TLS X.509 certificate environment in order to encrypt the data + channel. For the QEMU hypervisor, usage of a TLS environment can + be controlled on the host by the <code>vxhs_tls</code> and + <code>vxhs_tls_x509_cert_dir</code> or + <code>default_tls_x509_cert_dir</code> settings in the file + /etc/libvirt/qemu.conf. If <code>vxhs_tls</code> is enabled, + then unless the domain <code>tls</code> attribute is set to "no", + libvirt will use the host configured TLS environment. + If <code>vxhs_tls</code> is disabled, but the <code>tls</code> + attribute is set to "yes" in the device domain specification, + then libvirt will throw an error. + </p>

I see this is pretty much a copy of the chardev TLS wording; however, I'm not quite sure the exact same logic can apply in what you've done in this patch. Those last 3 lines may not be necessary - I guess that depends on what happens with how you handle or describe the "default" environment. Is there a reason you've added them and the check in the next patch? BTW: If they stay, it'd be the "domain disk source specification" to be specific.

</dd> <dt><code>snapshot</code></dt> <dd> diff --git a/src/qemu/libvirtd_qemu.aug b/src/qemu/libvirtd_qemu.aug index e1983d1..c19bf3a 100644 --- a/src/qemu/libvirtd_qemu.aug +++ b/src/qemu/libvirtd_qemu.aug @@ -115,6 +115,9 @@ module Libvirtd_qemu =

let memory_entry = str_entry "memory_backing_dir"

+ let vxhs_entry = bool_entry "vxhs_tls" + | str_entry "vxhs_tls_x509_cert_dir" + (* Each entry in the config is one of the following ... *) let entry = default_tls_entry | vnc_entry @@ -133,6 +136,7 @@ module Libvirtd_qemu = | nvram_entry | gluster_debug_level_entry | memory_entry + | vxhs_entry

let comment = [ label "#comment" . del /#[ \t]*/ "# " . store /([^ \t\n][^\n]*)?/ . del /\n/ "\n" ] let empty = [ label "#empty" . eol ] diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf index f977e3b..0bbcdb2 100644 --- a/src/qemu/qemu.conf +++ b/src/qemu/qemu.conf @@ -258,6 +258,29 @@ #chardev_tls_x509_secret_uuid = "00000000-0000-0000-0000-000000000000"

+# Enable use of TLS encryption on the VxHS network block devices. +# +# When the VxHS network block device server is set up appropriately, +# x509 certificates are used for authentication between the clients +# (qemu processes) and the remote VxHS server. +# +# It is necessary to setup CA and issue client and server certificates +# before enabling this. +# +#vxhs_tls = 1 + + +# In order to override the default TLS certificate location for VxHS +# device TCP certificates, supply a valid path to the certificate directory. +# This is used to authenticate the VxHS block device clients to the VxHS +# server. +# +# If the provided path does not exist then the default_tls_x509_cert_dir +# path will be used. +# +#vxhs_tls_x509_cert_dir = "/etc/pki/libvirt-vxhs" + +

I don't believe you can use/rely on default like the other definitions and that's something you're going to have to "call out" if you want to claim someone could use the default cert_dir. If the default environment didn't set _verify, then theoretically it won't have the client-cert.pem and client-key.pem files. What's not 100% clear is whether the VxHS server would require them. What is clear is that the QEMU block/vxhs.c code would provide them and that from what I've read iio_open() requires them. As for the default environment the _uuid value - that doesn't matter because it's used to decode the server.pem file and vxhs doesn't use that for the "client" side.

# In order to override the default TLS certificate location for migration # certificates, supply a valid path to the certificate directory. If the # provided path does not exist then the default_tls_x509_cert_dir path diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index ab5f7cc..3d53a86 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -283,6 +283,7 @@ virQEMUDriverConfigPtr virQEMUDriverConfigNew(bool privileged) SET_TLS_X509_CERT_DEFAULT(spice); SET_TLS_X509_CERT_DEFAULT(chardev); SET_TLS_X509_CERT_DEFAULT(migrate); + SET_TLS_X509_CERT_DEFAULT(vxhs);

This sets the default to /etc/pki/qemu if /etc/pki/libvirt-vxhs doesn't exist. The existence of /etc/pki/qemu is not checked until later...

#undef SET_TLS_X509_CERT_DEFAULT

@@ -380,6 +381,8 @@ static void virQEMUDriverConfigDispose(void *obj) VIR_FREE(cfg->chardevTLSx509certdir); VIR_FREE(cfg->chardevTLSx509secretUUID);

+ VIR_FREE(cfg->vxhsTLSx509certdir); + VIR_FREE(cfg->migrateTLSx509certdir); VIR_FREE(cfg->migrateTLSx509secretUUID);

@@ -556,6 +559,10 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg, goto cleanup; if (virConfGetValueBool(conf, "spice_auto_unix_socket", &cfg->spiceAutoUnixSocket) < 0) goto cleanup; + if (virConfGetValueBool(conf, "vxhs_tls", &cfg->vxhsTLS) < 0) + goto cleanup; + if (virConfGetValueString(conf, "vxhs_tls_x509_cert_dir", &cfg->vxhsTLSx509certdir) < 0) + goto cleanup;

In the time since you've last made changes in this space and now there's been some other adjustments which you'll need to add vxhs specific code for. See virQEMUDriverConfigTLSDirResetDefaults and add a CHECK_RESET_CERT_DIR_DEFAULT(vxhs); See virQEMUDriverConfigValidate to make the appropriate changes. If you've decided that you're going to allow usage of the default environment, then the verify code would have to ensure that if vxhs_tls were set that default verify was true. The default uuid has no bearing. If those conditions aren't met, then you'd have to fail the config setup. John

#define GET_CONFIG_TLS_CERTINFO(val) \ do { \ diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h index d469b50..13b6f81 100644 --- a/src/qemu/qemu_conf.h +++ b/src/qemu/qemu_conf.h @@ -203,6 +203,9 @@ struct _virQEMUDriverConfig { unsigned int glusterDebugLevel;

char *memoryBackingDir; + + bool vxhsTLS; + char *vxhsTLSx509certdir; };

/* Main driver state */ diff --git a/src/qemu/test_libvirtd_qemu.aug.in b/src/qemu/test_libvirtd_qemu.aug.in index 676d48c..688e5b9 100644 --- a/src/qemu/test_libvirtd_qemu.aug.in +++ b/src/qemu/test_libvirtd_qemu.aug.in @@ -25,6 +25,8 @@ module Test_libvirtd_qemu = { "chardev_tls_x509_cert_dir" = "/etc/pki/libvirt-chardev" } { "chardev_tls_x509_verify" = "1" } { "chardev_tls_x509_secret_uuid" = "00000000-0000-0000-0000-000000000000" } +{ "vxhs_tls" = "1" } +{ "vxhs_tls_x509_cert_dir" = "/etc/pki/libvirt-vxhs" } { "migrate_tls_x509_cert_dir" = "/etc/pki/libvirt-migrate" } { "migrate_tls_x509_verify" = "1" } { "migrate_tls_x509_secret_uuid" = "00000000-0000-0000-0000-000000000000" }

On 08/29/2017 02:39 AM, Ashish Mittal wrote:

The following describes the behavior of TLS for VxHS block device:

(1) Two new options have been added in /etc/libvirt/qemu.conf to control TLS behavior with VxHS block devices "vxhs_tls" and "vxhs_tls_x509_cert_dir". (2) Setting "vxhs_tls=1" in /etc/libvirt/qemu.conf will enable TLS for VxHS block devices. (3) "vxhs_tls_x509_cert_dir" can be set to the full path where the TLS certificates and keys are saved. If this value is missing,

TLS CA certificate and the client certificate and keys are saved.

the "default_tls_x509_cert_dir" will be used instead.

These three points I think belong in the previous patch commit message...

(4) If the value of "vxhs_tls" is set to 1, TLS creds will be added automatically on the qemu command line for every VxHS block device. (5) With "vxhs_tls=1", TLS may selectively be disabled on individual VxHS disks by specifying tls='no' in the device domain specification.

(4) and (5) somewhat contradict each other, but we can work through this. Additionally, let's use cfg->vxhsTLS or disk->source->haveTLS What this patch does is use the config->vxhsTLS along with the introduced disk->src->haveTLS in order to add TLS data to the VxHS client connection.

(6) Valid values for domain TLS setting are tls='yes|no'.

s/domain/domain disk source/

(7) tls='yes' can only be specified if "vxhs_tls" is enabled. Specifying tls='yes' when "vxhs_tls=0" results in an error.

I'm still not clear why this is a requirement if in fact you can really use the default qemu environment.

QEMU changes for VxHS (including TLS support) are already upstream.

Sample TLS args generated by libvirt - -object tls-creds-x509,id=objvxhs_tls0,dir=/usr/local/etc/pki/qemu,\ endpoint=client,verify-peer=yes \ -drive file.driver=vxhs,file.tls-creds=objvxhs_tls0,\ 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

Signed-off-by: Ashish Mittal <Ashish.Mittal@veritas.com> --- v5 changelog: (1) The v4 3/3 patch has been split into smaller chunks. (2) Functionally there are no changes in TLS code yet.

TODO: Changes to TLS functionality are pending.

docs/schemas/domaincommon.rng | 5 ++++ src/conf/domain_conf.c | 19 ++++++++++++ src/qemu/qemu_block.c | 42 +++++++++++++++++++-------- src/qemu/qemu_command.c | 67 +++++++++++++++++++++++++++++++++++++++++++ src/util/virstoragefile.c | 13 +++++++++ src/util/virstoragefile.h | 9 ++++++ 6 files changed, 143 insertions(+), 12 deletions(-)

So this patch is where things are going to get tricky. On one hand, trying to create a generic means to add/manage a 'tls' attribute for a disk source needs to be considered; however, this patch implements it strictly for VxHS, which may not be the best thing. Thus, I think there needs to be a patch prior to this that adds a generic/optional "tls" attribute to diskSourceNetworkHost. No one will use it yet, but it'll be there. Part of that patch would modify/introduce qemuDomainPrepareDiskSource to be run after qemuDomainPrepareChardevSource and before qemuDomainSecretPrepare. It's sole purpose would be to run through the disk array looking for disks to set up

diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 458b8d8..af38c9a 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -1651,6 +1651,11 @@ </attribute> <attribute name="name"/> <ref name="diskSourceNetworkHost"/> + <optional> + <attribute name="tls"> + <ref name="virYesNo"/> + </attribute> + </optional>

I think rather than here in the VxHS specific code - perhaps we add tls to the diskSourceNetworkHost. I need to investigate that. If that's done, then a patch before this one would be created.

</element> </define>

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 5bad397..f3fb3d0 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -8017,6 +8017,7 @@ virDomainDiskSourceParse(xmlNodePtr node, int ret = -1; char *protocol = NULL; xmlNodePtr saveNode = ctxt->node; + char *haveTLS = NULL;

ctxt->node = node;

@@ -8050,6 +8051,19 @@ virDomainDiskSourceParse(xmlNodePtr node, goto cleanup; }

+ /* Check tls=yes|no domain setting for the block device */ + /* At present only VxHS. Other block devices may be added later */

Adjust the above to be a multi-line comment... e.g. /* words * more words */ It's just a consistency thing.

+ if ((haveTLS = virXMLPropString(node, "tls")) && + src->protocol == VIR_STORAGE_NET_PROTOCOL_VXHS) {

flip-flop conditional - no need to check for it for other protocols (yet)... Besides that's a longer trip - let's make the quick check first.

+ if ((src->haveTLS = + virTristateBoolTypeFromString(haveTLS)) <= 0) { + virReportError(VIR_ERR_XML_ERROR, + _("unknown VxHS 'tls' setting '%s'"),

Adjust to remove "VxHS" since it could be generic some day... Replace with "disk source"

+ haveTLS); + goto cleanup; + } + } + /* for historical reasons the volume name for gluster volume is stored * as a part of the path. This is hard to work with when dealing with * relative names. Split out the volume into a separate variable */ @@ -8105,6 +8119,7 @@ virDomainDiskSourceParse(xmlNodePtr node,

cleanup: VIR_FREE(protocol); + VIR_FREE(haveTLS); ctxt->node = saveNode; return ret; } @@ -21534,6 +21549,10 @@ virDomainDiskSourceFormatNetwork(virBufferPtr buf,

VIR_FREE(path);

+ if (src->haveTLS != VIR_TRISTATE_BOOL_ABSENT) + virBufferAsprintf(buf, " tls='%s'", + virTristateBoolTypeToString(src->haveTLS)); + if (src->nhosts == 0 && !src->snapshot && !src->configFile) { virBufferAddLit(buf, "/>\n"); } else { diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c index a4d0160..766d07f 100644 --- a/src/qemu/qemu_block.c +++ b/src/qemu/qemu_block.c

And now we add in the qemu_alias.h include

@@ -519,20 +519,38 @@ qemuBlockStorageSourceGetVxHSProps(virStorageSourcePtr src) if (!(server = qemuBuildVxHSDriveJSONHost(src))) return NULL;

- /* VxHS disk specification example: - * { 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) { - virJSONValueFree(server); - ret = NULL; + if (src->addTLS == true) { + char *objalias = NULL; + + if (!(objalias = qemuAliasTLSObjFromSrcAlias("vxhs"))) + goto cleanup; + + if (virJSONValueObjectCreate(&ret, + "s:driver", protocol, + "s:tls-creds", objalias, + "s:vdisk-id", src->path, + "a:server", server, NULL) < 0) { + virJSONValueFree(server); + ret = NULL;

NB: The setting of ret = NULL on the error path was unnecessary

+ } + VIR_FREE(objalias); + } else { + /* VxHS disk specification example: + * { 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) { + virJSONValueFree(server); + ret = NULL; + }

Once you dig down into the depths of virJSONValueObjectCreate you'd find that virJSONValueObjectAddVArgs will allow passing a NULL argument by using a capital letter. If 'S:tls-creds' is used, then if tls-creds == NULL, then we don't print the string. So rather than the hunk above, we can modify the: if (virJSONValueObjectCreate(&ret, "s:driver", protocol, "s:vdisk-id", src->path, "a:server", server, NULL) < 0) { to be if (virJSONValueObjectCreate(&ret, "s:driver", protocol, "S:tls-creds", objalias, "s:vdisk-id", src->path, "a:server", server, NULL) < 0) { and only add: if (src->addTLS && !(objalias = qemuAliasTLSObjFromSrcAlias("vxhs"))) { virJSONValueFree(server); return NULL; } after the fetching of server. Whether "vxhs" will be unique enough remains to be thought about too.

}

+ cleanup:

and with all the above done correctly, cleanup: is unnecessary

return ret; }

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 0fd2674..384a489 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -791,6 +791,70 @@ qemuBuildTLSx509CommandLine(virCommandPtr cmd, }

+ +/* qemuBuildDiskVxHSTLSinfoCommandLine: + * @cmd: Pointer to the command string + * @cfg: Pointer to the qemu driver config + * @disk: The disk we are processing + * @qemuCaps: qemu capabilities object + * + * Check if the VxHS disk meets all the criteria to enable TLS. + * If yes, add a new TLS object and mention it's ID on the disk + * command line. + * + * Returns 0 on success, -1 w/ error on some sort of failure. + */ +static int +qemuBuildDiskVxHSTLSinfoCommandLine(virCommandPtr cmd, + virQEMUDriverConfigPtr cfg, + virDomainDiskDefPtr disk, + virQEMUCapsPtr qemuCaps) +{ + int ret = 0; + + if (cfg->vxhsTLS == true && disk->src->haveTLS != VIR_TRISTATE_BOOL_NO) {

s/ == true//

+ disk->src->addTLS = true;

Comparatively to chardev, see qemuDomainPrepareChardevSourceTLS Don't believe this will be necessary, the default being if not configured as "NO" then if the system configures it, it will be configured used, so no need to modify the domain to say you want it. I think we'll need to add that - especially if we ever consider hot plug/unplug.

+ ret = qemuBuildTLSx509CommandLine(cmd, cfg->vxhsTLSx509certdir, + false, + true,

By setting this to true here requires that the environment that was configured will have the server verify the client as per the various verify descriptions in qemu.conf.

+ false, + "vxhs", + qemuCaps);

There's some extraneous indent/spacing issues...

+ } else if (cfg->vxhsTLS == false &&

s/cfg->vxhsTLS == false &&/!cfg->vxhsTLS/

+ disk->src->haveTLS == VIR_TRISTATE_BOOL_YES) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("Please enable VxHS specific TLS options in the qemu " + "conf file before using TLS in VxHS device domain " + "specification")); + ret = -1;

And this begs the question why? It's the whole reason for a "default" environment. The error message is also way too long... Still this type of check is better left to *Prepare* type logic. The more recent "desire" in the qemu_command code is not fail on configuration errors, but fail only on command generation errors.

+ } + + return ret; +} + + +/* qemuBuildDiskTLSinfoCommandLine: + * + * Add TLS object if the disk uses a secure communication channel + * + * Returns 0 on success, -1 w/ error on some sort of failure. + */ +static int +qemuBuildDiskTLSinfoCommandLine(virCommandPtr cmd, + virQEMUDriverConfigPtr cfg, + virDomainDiskDefPtr disk, + virQEMUCapsPtr qemuCaps) +{ + virStorageSourcePtr src = disk->src; + + /* other protocols may be added later */ + if (src->protocol == VIR_STORAGE_NET_PROTOCOL_VXHS) + return qemuBuildDiskVxHSTLSinfoCommandLine(cmd, cfg, disk, qemuCaps); + + return 0; +} + + static char * qemuBuildNetworkDriveURI(virStorageSourcePtr src, qemuDomainSecretInfoPtr secinfo) @@ -2218,6 +2282,9 @@ qemuBuildDiskDriveCommandLine(virCommandPtr cmd, if (qemuBuildDiskSecinfoCommandLine(cmd, encinfo) < 0) return -1;

+ if (qemuBuildDiskTLSinfoCommandLine(cmd, cfg, disk, qemuCaps) < 0) + return -1; + virCommandAddArg(cmd, "-drive");

if (!(optstr = qemuBuildDriveStr(disk, cfg, driveBoot, qemuCaps))) diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index e9a59e0..d4f0fdb 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -2039,6 +2039,8 @@ virStorageSourceCopy(const virStorageSource *src, ret->physical = src->physical; ret->readonly = src->readonly; ret->shared = src->shared; + ret->haveTLS = src->haveTLS; + ret->addTLS = src->addTLS;

/* storage driver metadata are not copied */ ret->drv = NULL; @@ -3219,6 +3221,7 @@ virStorageSourceParseBackingJSONVxHS(virStorageSourcePtr src, { const char *vdisk_id = virJSONValueObjectGetString(json, "vdisk-id"); virJSONValuePtr server = virJSONValueObjectGetObject(json, "server"); + const char *haveTLS = virJSONValueObjectGetString(json, "tls");

if (!vdisk_id || !server) { virReportError(VIR_ERR_INVALID_ARG, "%s", @@ -3227,6 +3230,16 @@ virStorageSourceParseBackingJSONVxHS(virStorageSourcePtr src, return -1; }

+ if (haveTLS) { + if ((src->haveTLS = + virTristateBoolTypeFromString(haveTLS)) <= 0) { + virReportError(VIR_ERR_INVALID_ARG, + _("unknown VxHS 'tls' setting '%s'"), + haveTLS); + return -1; + } + } + src->type = VIR_STORAGE_TYPE_NETWORK; src->protocol = VIR_STORAGE_NET_PROTOCOL_VXHS;

diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index f7e897f..0f363a7 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -281,6 +281,15 @@ struct _virStorageSource { /* metadata that allows identifying given storage source */ char *nodeformat; /* name of the format handler object */ char *nodestorage; /* name of the storage object */ + + /* This is the domain specific setting. + * It may be absent */ + int haveTLS; /* enum virTristateBool */ + + /* This should be set to "true" only when TLS creds are to be added for + * the device. For e.g. this could be based on a combination of + * global conf setting + domain specific setting */ + bool addTLS;

Not convinced yet 'addTLS' is needed - need to do some more thinking on this. This has already been a day long exercise - so I'll stop at this patch. I think we can get everything through the TLS changes merged into libvirt upstream once 3.8.0 opens for business. Whether more is ready depends on how much progress we can make. John

};

On 08/29/2017 02:39 AM, Ashish Mittal wrote:

Enable TLS for VxHS disks in qemu.conf and ensure that TLS arguments are added automatically on the qemu command line.

Signed-off-by: Ashish Mittal <Ashish.Mittal@veritas.com> --- ...muxml2argv-disk-drive-network-tlsx509-vxhs.args | 30 +++++++++++++++++++ ...emuxml2argv-disk-drive-network-tlsx509-vxhs.xml | 34 ++++++++++++++++++++++ tests/qemuxml2argvtest.c | 5 ++++ 3 files changed, 69 insertions(+) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-tlsx509-vxhs.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-tlsx509-vxhs.xml

This should also add a qemuxml2xmltest.c adjustment (similar to earlier)

diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-tlsx509-vxhs.args b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-tlsx509-vxhs.args new file mode 100644 index 0000000..e872042 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-tlsx509-vxhs.args @@ -0,0 +1,30 @@ +LC_ALL=C \ +PATH=/bin \ +HOME=/home/test \ +USER=test \ +LOGNAME=test \ +QEMU_AUDIO_DRV=none \ +/usr/bin/qemu-system-x86_64 \ +-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 \ +-chardev socket,id=charmonitor,path=/tmp/lib/domain--1-QEMUGuest1/monitor.sock,\ +server,nowait \ +-mon chardev=charmonitor,id=monitor,mode=readline \ +-no-acpi \ +-boot c \ +-usb \ +-object tls-creds-x509,id=objvxhs_tls0,dir=/usr/local/etc/pki/qemu,\

dir=/etc/pki/qemu

+endpoint=client,verify-peer=yes \ +-drive file.driver=vxhs,file.tls-creds=objvxhs_tls0,\ +file.vdisk-id=eb90327c-8302-4725-9e1b-4e85ed4dc251,\ +file.server.host=192.168.0.1,file.server.port=9999,format=raw,if=none,\

these will be file.server.0.{host|port} as well as adding the .type

+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-tlsx509-vxhs.xml b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-tlsx509-vxhs.xml new file mode 100644 index 0000000..a488770 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-tlsx509-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/bin/qemu-system-x86_64</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/>

Remove this

+ <target dev='vda' bus='virtio'/> + <serial>eb90327c-8302-4725-9e1b-4e85ed4dc251</serial> + <alias name='virtio-disk0'/>

and this John

+ <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 f7dbf39..57b838d 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -932,6 +932,11 @@ mymain(void) 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); + driver.config->vxhsTLS = 1; + DO_TEST("disk-drive-network-tlsx509-vxhs", + QEMU_CAPS_OBJECT_TLS_CREDS_X509); + driver.config->vxhsTLS = 0; + VIR_FREE(driver.config->vxhsTLSx509certdir); DO_TEST("disk-drive-no-boot", QEMU_CAPS_BOOTINDEX); DO_TEST_PARSE_ERROR("disk-device-lun-type-invalid",

On 08/29/2017 02:39 AM, Ashish Mittal wrote:

Test case verifies that XML is generated correctly for a VxHS disk having TLS enabled.

Signed-off-by: Ashish Mittal <Ashish.Mittal@veritas.com> --- tests/virstoragetest.c | 11 +++++++++++ 1 file changed, 11 insertions(+)

diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c index f8444e1..92328ea 100644 --- a/tests/virstoragetest.c +++ b/tests/virstoragetest.c @@ -1595,6 +1595,17 @@ mymain(void) "<source protocol='vxhs' name='c6718f6b-0401-441d-a8c3-1f0064d75ee0'>\n" " <host name='example.com' port='1234'/>\n" "</source>\n"); + TEST_BACKING_PARSE("json:{\"file\":{\"driver\":\"vxhs\"," + "\"vdisk-id\":\"c6718f6b-0401-441d-a8c3-1f0064d75ee0\"," + "\"server\": { \"host\":\"example.com\"," + "\"port\":\"1234\""

Similar to previous alter to have "type":"tcp" and use 9999 for port. Although this does work as is. John

+ "}," + "\"tls\":\"yes\"" + "}" + "}", + "<source protocol='vxhs' name='c6718f6b-0401-441d-a8c3-1f0064d75ee0' tls='yes'>\n" + " <host name='example.com' port='1234'/>\n" + "</source>\n"); #endif /* WITH_YAJL */

cleanup:

On 08/29/2017 02:39 AM, Ashish Mittal wrote:

Add a test case to verify that if vxhs_tls is disabled in qemu.conf then adding "tls=yes" in the domain XML generates an error.

Signed-off-by: Ashish Mittal <Ashish.Mittal@veritas.com> --- ...ml2argv-disk-drive-network-tlsx509-err-vxhs.xml | 34 ++++++++++++++++++++++ tests/qemuxml2argvtest.c | 2 ++ 2 files changed, 36 insertions(+) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-tlsx509-err-vxhs.xml

While this works as advertised, I'm not clear why it matters. It's something we can talk about/debate once I repost the series. I'll leave it in the next posting, but I'm not sure it's desired. John

diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-tlsx509-err-vxhs.xml b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-tlsx509-err-vxhs.xml new file mode 100644 index 0000000..951ad4e --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-tlsx509-err-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/bin/qemu-system-x86_64</emulator> + <disk type='network' device='disk'> + <driver name='qemu' type='raw' cache='none'/> + <source protocol='vxhs' name='eb90327c-8302-4725-9e1b-4e85ed4dc251' tls='yes'> + <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 57b838d..d9723c4 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -932,6 +932,8 @@ mymain(void) 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-tlsx509-err-vxhs", + QEMU_CAPS_OBJECT_TLS_CREDS_X509); driver.config->vxhsTLS = 1; DO_TEST("disk-drive-network-tlsx509-vxhs", QEMU_CAPS_OBJECT_TLS_CREDS_X509);

On 08/29/2017 02:39 AM, Ashish Mittal wrote:

Verifies TLS args are auto generated if enabled in qemu.conf Verifies TLS args are not generated if XML specifies tls='no'

Signed-off-by: Ashish Mittal <Ashish.Mittal@veritas.com> --- ...-disk-drive-network-tlsx509-multidisk-vxhs.args | 43 +++++++++++++++++ ...v-disk-drive-network-tlsx509-multidisk-vxhs.xml | 56 ++++++++++++++++++++++ tests/qemuxml2argvtest.c | 2 + 3 files changed, 101 insertions(+) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-tlsx509-multidisk-vxhs.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-tlsx509-multidisk-vxhs.xml

diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-tlsx509-multidisk-vxhs.args b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-tlsx509-multidisk-vxhs.args new file mode 100644 index 0000000..005ad78 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-tlsx509-multidisk-vxhs.args @@ -0,0 +1,43 @@ +LC_ALL=C \ +PATH=/bin \ +HOME=/home/test \ +USER=test \ +LOGNAME=test \ +QEMU_AUDIO_DRV=none \ +/usr/bin/qemu-system-x86_64 \ +-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 \ +-chardev socket,id=charmonitor,path=/tmp/lib/domain--1-QEMUGuest1/monitor.sock,\ +server,nowait \ +-mon chardev=charmonitor,id=monitor,mode=readline \ +-no-acpi \ +-boot c \ +-usb \ +-object tls-creds-x509,id=objvxhs_tls0,dir=/usr/local/etc/pki/qemu,\

dir=/etc/pki/qemu

+endpoint=client,verify-peer=yes \ +-drive file.driver=vxhs,file.tls-creds=objvxhs_tls0,\ +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 \ +-object tls-creds-x509,id=objvxhs_tls0,dir=/usr/local/etc/pki/qemu,\

dir=/etc/pki/qemu But this points out a problem - you now have two objects using the same id "id=objvxhs_tls0". This would fail a real qemu start... So here's my suggestion - see that "drive-virtio-disk0" - let's make use of that and instead of "objvxhs_tls0" - this would become "objvirtio-disk0-tls" - it's generic. This of course goes back to patch 5 where the alias was first created...

+endpoint=client,verify-peer=yes \ +-drive file.driver=vxhs,file.tls-creds=objvxhs_tls0,\ +file.vdisk-id=eb90327c-8302-4725-9e1b-4e85ed4dc252,\ +file.server.host=192.168.0.2,file.server.port=9999,format=raw,if=none,\ +id=drive-virtio-disk1,cache=none \ +-device virtio-blk-pci,bus=pci.0,addr=0x5,drive=drive-virtio-disk1,\ +id=virtio-disk1 \ +-drive file.driver=vxhs,file.vdisk-id=eb90327c-8302-4725-9e1b-4e85ed4dc253,\ +file.server.host=192.168.0.3,file.server.port=9999,format=raw,if=none,\ +id=drive-virtio-disk2,cache=none \ +-device virtio-blk-pci,bus=pci.0,addr=0x6,drive=drive-virtio-disk2,\ +id=virtio-disk2

Need to alter the various outputs to include the file.server.0.type and then file.server.0 for host and port

diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-tlsx509-multidisk-vxhs.xml b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-tlsx509-multidisk-vxhs.xml new file mode 100644 index 0000000..3d28958 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-tlsx509-multidisk-vxhs.xml @@ -0,0 +1,56 @@ +<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/bin/qemu-system-x86_64</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/>

Remove...

+ <target dev='vda' bus='virtio'/> + <serial>eb90327c-8302-4725-9e1b-4e85ed4dc251</serial> + <alias name='virtio-disk0'/>

Remove...

+ <address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x0'/> + </disk> + <disk type='network' device='disk'> + <driver name='qemu' type='raw' cache='none'/> + <source protocol='vxhs' name='eb90327c-8302-4725-9e1b-4e85ed4dc252'> + <host name='192.168.0.2' port='9999'/> + </source> + <backingStore/>

Remove...

+ <target dev='vdb' bus='virtio'/> + <serial>eb90327c-8302-4725-9e1b-4e85ed4dc252</serial> + <alias name='virtio-disk0'/>

!!!! Same as other one!!! This is virtio-disk1, but still it's unnecessary, so it needs to be removed.

+ <address type='pci' domain='0x0000' bus='0x00' slot='0x05' function='0x0'/> + </disk> + <disk type='network' device='disk'> + <driver name='qemu' type='raw' cache='none'/> + <source protocol='vxhs' name='eb90327c-8302-4725-9e1b-4e85ed4dc253' tls='no'> + <host name='192.168.0.3' port='9999'/> + </source> + <backingStore/>

Remove

+ <target dev='vdc' bus='virtio'/> + <serial>eb90327c-8302-4725-9e1b-4e85ed4dc252</serial> + <alias name='virtio-disk0'/>

!! would be virtio-disk2, but remove it. As you can see - I've gone through everything now. While doing so I've been making changes to sources and patch order a bit. I'll clean that all up and post in a little while. I need to go through each patch and figure out what I changed in order to "call it out"... John

+ <address type='pci' domain='0x0000' bus='0x00' slot='0x06' 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 d9723c4..bc9d3a2 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -937,6 +937,8 @@ mymain(void) driver.config->vxhsTLS = 1; DO_TEST("disk-drive-network-tlsx509-vxhs", QEMU_CAPS_OBJECT_TLS_CREDS_X509); + DO_TEST("disk-drive-network-tlsx509-multidisk-vxhs", + QEMU_CAPS_OBJECT_TLS_CREDS_X509); driver.config->vxhsTLS = 0; VIR_FREE(driver.config->vxhsTLSx509certdir); DO_TEST("disk-drive-no-boot",