[libvirt] [PATCH 00/13] Network disk improvements (NBD & libiscsi)
This series improves support for NBD disks (patches 1-6), and adds support for the libiscsi userspace initiator (patches 7-13). Please review! Paolo Paolo Bonzini (13): qemu: fix use-after-free when parsing NBD disk qemu: do not support non-network disks without -drive qemu: rewrite NBD command-line builder and parser qemu: support named nbd exports qemu: support NBD with Unix sockets qemu: support URI syntax for NBD domain: add support for iscsi network disks qemu: add support for libiscsi qemu: support LUN numbers for iSCSI disks domain: make port optional for network disks secret: add iscsi to possible usage types domain: parse XML for iscsi authorization credentials qemu: pass iscsi authorization credentials docs/formatdomain.html.in | 42 +- docs/formatsecret.html.in | 12 + docs/schemas/domaincommon.rng | 37 +- docs/schemas/secret.rng | 10 + include/libvirt/libvirt.h.in | 1 + src/conf/domain_conf.c | 51 ++- src/conf/domain_conf.h | 3 + src/conf/secret_conf.c | 22 +- src/conf/secret_conf.h | 1 + src/qemu/qemu_command.c | 432 ++++++++++++++------- src/secret/secret_driver.c | 8 + tests/qemuargv2xmltest.c | 5 + .../qemuxml2argv-disk-drive-network-gluster.args | 2 +- ...qemuxml2argv-disk-drive-network-iscsi-auth.args | 1 + .../qemuxml2argv-disk-drive-network-iscsi-auth.xml | 31 ++ .../qemuxml2argv-disk-drive-network-iscsi.args | 1 + .../qemuxml2argv-disk-drive-network-iscsi.xml | 34 ++ ...qemuxml2argv-disk-drive-network-nbd-export.args | 5 + .../qemuxml2argv-disk-drive-network-nbd-export.xml | 33 ++ ...ml2argv-disk-drive-network-nbd-ipv6-export.args | 5 + ...xml2argv-disk-drive-network-nbd-ipv6-export.xml | 33 ++ .../qemuxml2argv-disk-drive-network-nbd-ipv6.args | 5 + .../qemuxml2argv-disk-drive-network-nbd-ipv6.xml | 33 ++ .../qemuxml2argv-disk-drive-network-nbd-unix.args | 5 + .../qemuxml2argv-disk-drive-network-nbd-unix.xml | 33 ++ tests/qemuxml2argvtest.c | 12 + tests/qemuxml2xmltest.c | 7 + 27 files changed, 687 insertions(+), 177 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi-auth.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi-auth.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-nbd-export.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-nbd-export.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-nbd-ipv6-export.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-nbd-ipv6-export.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-nbd-ipv6.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-nbd-ipv6.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-nbd-unix.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-nbd-unix.xml -- 1.8.1.2
disk->src is still used for disks->hosts->name, do not free it. Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- src/qemu/qemu_command.c | 1 - 1 file changed, 1 deletion(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index dee493f..5dccaae 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -8707,7 +8707,6 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr qemuCaps, disk->hosts->port = strdup(port); if (!disk->hosts->port) goto no_memory; - VIR_FREE(disk->src); disk->src = NULL; break; case VIR_DOMAIN_DISK_PROTOCOL_RBD: -- 1.8.1.2
On 02/25/2013 10:44 AM, Paolo Bonzini wrote:
disk->src is still used for disks->hosts->name, do not free it.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- src/qemu/qemu_command.c | 1 - 1 file changed, 1 deletion(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index dee493f..5dccaae 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -8707,7 +8707,6 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr qemuCaps,
A bit more context helps: char *host, *port; host = disk->src; disk->hosts->name = host;
disk->hosts->port = strdup(port); if (!disk->hosts->port) goto no_memory; - VIR_FREE(disk->src); disk->src = NULL; break;
So there is definitely a use-after-free bug fixed by your patch. However, your patch causes a double-free bug on error (if the strdup(port) fails, then disk->hosts->port and disk->src are the same pointer, but we attempt to free both of them). I'm squashing this in before pushing, to make the transfer semantics more obvious: diff --git i/src/qemu/qemu_command.c w/src/qemu/qemu_command.c index 0a7d4ec..f8f3ade 100644 --- i/src/qemu/qemu_command.c +++ w/src/qemu/qemu_command.c @@ -8832,11 +8832,11 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr qemuCaps, if (VIR_ALLOC(disk->hosts) < 0) goto no_memory; disk->nhosts = 1; - disk->hosts->name = host; + disk->hosts->name = disk->src; + disk->src = NULL; disk->hosts->port = strdup(port); if (!disk->hosts->port) goto no_memory; - disk->src = NULL; break; case VIR_DOMAIN_DISK_PROTOCOL_RBD: /* old-style CEPH_ARGS env variable is parsed later */ -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 02/27/2013 10:03 PM, Eric Blake wrote:
So there is definitely a use-after-free bug fixed by your patch. However, your patch causes a double-free bug on error (if the
Rather, your patch did nothing to address the pre-existing double-free bug on error. Guess I should be more careful when pinning the blame :)
strdup(port) fails, then disk->hosts->port and disk->src are the same pointer, but we attempt to free both of them).
-- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Il 28/02/2013 06:03, Eric Blake ha scritto:
diff --git i/src/qemu/qemu_command.c w/src/qemu/qemu_command.c index 0a7d4ec..f8f3ade 100644 --- i/src/qemu/qemu_command.c +++ w/src/qemu/qemu_command.c @@ -8832,11 +8832,11 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr qemuCaps, if (VIR_ALLOC(disk->hosts) < 0) goto no_memory; disk->nhosts = 1; - disk->hosts->name = host; + disk->hosts->name = disk->src; + disk->src = NULL; disk->hosts->port = strdup(port); if (!disk->hosts->port) goto no_memory; - disk->src = NULL; break; case VIR_DOMAIN_DISK_PROTOCOL_RBD: /* old-style CEPH_ARGS env variable is parsed later */
ACK Paolo
QEMU added -drive in 2007, and NBD in 2008. Both appeared first in release 0.10.0. Thus the code to support network disks without -drive is dead, and in fact it incorrectly escapes commas. Drop it. Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- src/qemu/qemu_command.c | 53 ++----------------------------------------------- 1 file changed, 2 insertions(+), 51 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 5dccaae..a3c5a4e 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -5901,57 +5901,8 @@ qemuBuildCommandLine(virConnectPtr conn, goto no_memory; } } else if (disk->type == VIR_DOMAIN_DISK_TYPE_NETWORK) { - switch (disk->protocol) { - case VIR_DOMAIN_DISK_PROTOCOL_NBD: - if (disk->nhosts != 1) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("NBD accepts only one host")); - goto error; - } - if (virAsprintf(&file, "nbd:%s:%s,", disk->hosts->name, - disk->hosts->port) < 0) { - goto no_memory; - } - break; - case VIR_DOMAIN_DISK_PROTOCOL_RBD: - { - virBuffer opt = VIR_BUFFER_INITIALIZER; - if (qemuBuildRBDString(conn, disk, &opt) < 0) - goto error; - if (virBufferError(&opt)) { - virReportOOMError(); - goto error; - } - file = virBufferContentAndReset(&opt); - } - break; - case VIR_DOMAIN_DISK_PROTOCOL_GLUSTER: - { - virBuffer opt = VIR_BUFFER_INITIALIZER; - if (qemuBuildGlusterString(disk, &opt) < 0) - goto error; - if (virBufferError(&opt)) { - virReportOOMError(); - goto error; - } - file = virBufferContentAndReset(&opt); - } - break; - case VIR_DOMAIN_DISK_PROTOCOL_SHEEPDOG: - if (disk->nhosts == 0) { - if (virAsprintf(&file, "sheepdog:%s,", disk->src) < 0) { - goto no_memory; - } - } else { - /* only one host is supported now */ - if (virAsprintf(&file, "sheepdog:%s:%s:%s,", - disk->hosts->name, disk->hosts->port, - disk->src) < 0) { - goto no_memory; - } - } - break; - } + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("network disks are only supported with -drive")); } else { if (!(file = strdup(disk->src))) { goto no_memory; -- 1.8.1.2
On 2013年02月26日 01:44, Paolo Bonzini wrote:
QEMU added -drive in 2007, and NBD in 2008. Both appeared first in release 0.10.0. Thus the code to support network disks without -drive is dead, and in fact it incorrectly escapes commas. Drop it.
The network disks support appeared in 0.8.7: Jan 4 2011 actually. commit 036ad5052b43fe9f0d197e89fd16715950408e1d Author: MORITA Kazutaka <morita.kazutaka@lab.ntt.co.jp> Date: Mon Dec 6 16:24:09 2010 +0900 add network disk support This patch adds network disk support to libvirt/QEMU. The currently supported protocols are nbd, rbd, and sheepdog. The XML syntax is like this: <disk type="network" device="disk"> <driver name="qemu" type="raw" /> <source protocol='rbd|sheepdog|nbd' name="...some image identifier..."> <host name="mon1.example.org" port="6000"> <host name="mon2.example.org" port="6000"> <host name="mon3.example.org" port="6000"> </source> <target dev="vda" bus="virtio" /> </disk> Signed-off-by: MORITA Kazutaka <morita.kazutaka@lab.ntt.co.jp> I believe -drive support is before 0.8.7, but how about libvirt is new enough with the network disk support, and qemu too old without -drive support? Osier
Il 11/03/2013 04:23, Osier Yang ha scritto:
On 2013年02月26日 01:44, Paolo Bonzini wrote:
QEMU added -drive in 2007, and NBD in 2008. Both appeared first in release 0.10.0. Thus the code to support network disks without -drive is dead, and in fact it incorrectly escapes commas. Drop it.
The network disks support appeared in 0.8.7: Jan 4 2011 actually.
commit 036ad5052b43fe9f0d197e89fd16715950408e1d Author: MORITA Kazutaka <morita.kazutaka@lab.ntt.co.jp> Date: Mon Dec 6 16:24:09 2010 +0900
add network disk support
This patch adds network disk support to libvirt/QEMU. The currently supported protocols are nbd, rbd, and sheepdog. The XML syntax is like this:
<disk type="network" device="disk"> <driver name="qemu" type="raw" /> <source protocol='rbd|sheepdog|nbd' name="...some image identifier..."> <host name="mon1.example.org" port="6000"> <host name="mon2.example.org" port="6000"> <host name="mon3.example.org" port="6000"> </source> <target dev="vda" bus="virtio" /> </disk>
Signed-off-by: MORITA Kazutaka <morita.kazutaka@lab.ntt.co.jp>
I believe -drive support is before 0.8.7, but how about libvirt is new enough with the network disk support, and qemu too old without -drive support?
Sorry for the confusion---I meant both -drive and NBD were in QEMU 0.10.0. There is no released QEMU version that has only one of -drive and NBD, and not even a source control commit that has NBD but not -drive. Paolo
On 2013年03月11日 21:16, Paolo Bonzini wrote:
Il 11/03/2013 04:23, Osier Yang ha scritto:
On 2013年02月26日 01:44, Paolo Bonzini wrote:
QEMU added -drive in 2007, and NBD in 2008. Both appeared first in release 0.10.0. Thus the code to support network disks without -drive is dead, and in fact it incorrectly escapes commas. Drop it.
The network disks support appeared in 0.8.7: Jan 4 2011 actually.
commit 036ad5052b43fe9f0d197e89fd16715950408e1d Author: MORITA Kazutaka<morita.kazutaka@lab.ntt.co.jp> Date: Mon Dec 6 16:24:09 2010 +0900
add network disk support
This patch adds network disk support to libvirt/QEMU. The currently supported protocols are nbd, rbd, and sheepdog. The XML syntax is like this:
<disk type="network" device="disk"> <driver name="qemu" type="raw" /> <source protocol='rbd|sheepdog|nbd' name="...some image identifier..."> <host name="mon1.example.org" port="6000"> <host name="mon2.example.org" port="6000"> <host name="mon3.example.org" port="6000"> </source> <target dev="vda" bus="virtio" /> </disk>
Signed-off-by: MORITA Kazutaka<morita.kazutaka@lab.ntt.co.jp>
I believe -drive support is before 0.8.7, but how about libvirt is new enough with the network disk support, and qemu too old without -drive support?
Sorry for the confusion---I meant both -drive and NBD were in QEMU 0.10.0. There is no released QEMU version that has only one of -drive and NBD, and not even a source control commit that has NBD but not -drive.
It makes sense then, ACK.
On Mon, Feb 25, 2013 at 06:44:21PM +0100, Paolo Bonzini wrote:
QEMU added -drive in 2007, and NBD in 2008. Both appeared first in release 0.10.0. Thus the code to support network disks without -drive is dead, and in fact it incorrectly escapes commas. Drop it.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- src/qemu/qemu_command.c | 53 ++----------------------------------------------- 1 file changed, 2 insertions(+), 51 deletions(-)
ACK Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 03/15/2013 08:29 AM, Daniel P. Berrange wrote:
On Mon, Feb 25, 2013 at 06:44:21PM +0100, Paolo Bonzini wrote:
QEMU added -drive in 2007, and NBD in 2008. Both appeared first in release 0.10.0. Thus the code to support network disks without -drive is dead, and in fact it incorrectly escapes commas. Drop it.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- src/qemu/qemu_command.c | 53 ++----------------------------------------------- 1 file changed, 2 insertions(+), 51 deletions(-)
ACK
Pushed. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Il 15/03/2013 15:43, Eric Blake ha scritto:
On 03/15/2013 08:29 AM, Daniel P. Berrange wrote:
On Mon, Feb 25, 2013 at 06:44:21PM +0100, Paolo Bonzini wrote:
QEMU added -drive in 2007, and NBD in 2008. Both appeared first in release 0.10.0. Thus the code to support network disks without -drive is dead, and in fact it incorrectly escapes commas. Drop it.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- src/qemu/qemu_command.c | 53 ++----------------------------------------------- 1 file changed, 2 insertions(+), 51 deletions(-)
ACK
Pushed.
Ok to push 3-10 and 14 too? Then I'll send the coding style adjustments that Osier requested (but Dan acked nevertheless) and 11-13 which need a bit more work. Paolo
On Fri, Mar 15, 2013 at 04:56:26PM +0100, Paolo Bonzini wrote:
Il 15/03/2013 15:43, Eric Blake ha scritto:
On 03/15/2013 08:29 AM, Daniel P. Berrange wrote:
On Mon, Feb 25, 2013 at 06:44:21PM +0100, Paolo Bonzini wrote:
QEMU added -drive in 2007, and NBD in 2008. Both appeared first in release 0.10.0. Thus the code to support network disks without -drive is dead, and in fact it incorrectly escapes commas. Drop it.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- src/qemu/qemu_command.c | 53 ++----------------------------------------------- 1 file changed, 2 insertions(+), 51 deletions(-)
ACK
Pushed.
Ok to push 3-10 and 14 too? Then I'll send the coding style adjustments that Osier requested (but Dan acked nevertheless) and 11-13 which need a bit more work.
Nope, not ok to push 7 or 8, given the way it represents IQN & LUN as separate attributes. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 03/15/2013 10:17 AM, Daniel P. Berrange wrote:
Ok to push 3-10 and 14 too? Then I'll send the coding style adjustments that Osier requested (but Dan acked nevertheless) and 11-13 which need a bit more work.
Nope, not ok to push 7 or 8, given the way it represents IQN & LUN as separate attributes.
I've applied 3-6, but now I'm lost. I didn't see negative comments on 7 or 8 - but did see a complaint on 9 which in turn had reply that 8 needs a respin to not forbid '/'. To play it safe, I will wait for a v2 respin of all remaining patches. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Move the code to an external function, and structure it to prepare the addition of new features in the next few patches. Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- src/qemu/qemu_command.c | 128 ++++++++++++++++++++++++++++-------------------- tests/qemuxml2xmltest.c | 1 + 2 files changed, 76 insertions(+), 53 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index a3c5a4e..beb7cfe 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -2128,6 +2128,45 @@ error: } static int +qemuParseNBDString(virDomainDiskDefPtr disk) +{ + virDomainDiskHostDefPtr h = NULL; + char *host, *port; + + if (VIR_ALLOC(h) < 0) + goto no_memory; + + host = disk->src + strlen("nbd:"); + port = strchr(host, ':'); + if (!port) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("cannot parse nbd filename '%s'"), disk->src); + goto error; + } + + *port++ = '\0'; + h->name = strdup(host); + if (!h->name) + goto no_memory; + + h->port = strdup(port); + if (!h->port) + goto no_memory; + + VIR_FREE(disk->src); + disk->nhosts = 1; + disk->hosts = h; + return 0; + +no_memory: + virReportOOMError(); +error: + virDomainDiskHostDefFree(h); + VIR_FREE(h); + return -1; +} + +static int qemuBuildGlusterString(virDomainDiskDefPtr disk, virBufferPtr opt) { int ret = -1; @@ -2188,6 +2227,36 @@ no_memory: goto cleanup; } +static int +qemuBuildNBDString(virDomainDiskDefPtr disk, virBufferPtr opt) +{ + const char *transp; + + if (disk->nhosts != 1) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("nbd accepts only one host")); + return -1; + } + + virBufferAddLit(opt, "file=nbd:"); + + switch (disk->hosts->transport) { + case VIR_DOMAIN_DISK_PROTO_TRANS_TCP: + if (disk->hosts->name) + virBufferEscape(opt, ',', ",", "%s", disk->hosts->name); + virBufferEscape(opt, ',', ",", ":%s", + disk->hosts->port ? disk->hosts->port : "10809"); + break; + default: + transp = virDomainDiskProtocolTransportTypeToString(disk->hosts->transport); + virReportError(VIR_ERR_INTERNAL_ERROR, + _("nbd does not support transport '%s'"), transp); + break; + } + + return 0; +} + char * qemuBuildDriveStr(virConnectPtr conn ATTRIBUTE_UNUSED, virDomainDiskDefPtr disk, @@ -2314,13 +2383,9 @@ qemuBuildDriveStr(virConnectPtr conn ATTRIBUTE_UNUSED, } else if (disk->type == VIR_DOMAIN_DISK_TYPE_NETWORK) { switch (disk->protocol) { case VIR_DOMAIN_DISK_PROTOCOL_NBD: - if (disk->nhosts != 1) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("NBD accepts only one host")); + if (qemuBuildNBDString(disk, &opt) < 0) goto error; - } - virBufferAsprintf(&opt, "file=nbd:%s:%s,", - disk->hosts->name, disk->hosts->port); + virBufferAddChar(&opt, ','); break; case VIR_DOMAIN_DISK_PROTOCOL_RBD: virBufferAddLit(&opt, "file="); @@ -7337,39 +7402,11 @@ qemuParseCommandLineDisk(virCapsPtr qemuCaps, if (STRPREFIX(def->src, "/dev/")) def->type = VIR_DOMAIN_DISK_TYPE_BLOCK; else if (STRPREFIX(def->src, "nbd:")) { - char *host, *port; - def->type = VIR_DOMAIN_DISK_TYPE_NETWORK; def->protocol = VIR_DOMAIN_DISK_PROTOCOL_NBD; - host = def->src + strlen("nbd:"); - port = strchr(host, ':'); - if (!port) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("cannot parse nbd filename '%s'"), - def->src); - goto error; - } - *port++ = '\0'; - if (VIR_ALLOC(def->hosts) < 0) { - virReportOOMError(); - goto error; - } - def->nhosts = 1; - def->hosts->name = strdup(host); - if (!def->hosts->name) { - virReportOOMError(); - goto error; - } - def->hosts->port = strdup(port); - if (!def->hosts->port) { - virReportOOMError(); - goto error; - } - def->hosts->transport = VIR_DOMAIN_DISK_PROTO_TRANS_TCP; - def->hosts->socket = NULL; - VIR_FREE(def->src); - def->src = NULL; + if (qemuParseNBDString(def) < 0) + goto error; } else if (STRPREFIX(def->src, "rbd:")) { char *p = def->src; @@ -8599,7 +8636,6 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr qemuCaps, else if (STRPREFIX(val, "nbd:")) { disk->type = VIR_DOMAIN_DISK_TYPE_NETWORK; disk->protocol = VIR_DOMAIN_DISK_PROTOCOL_NBD; - val += strlen("nbd:"); } else if (STRPREFIX(val, "rbd:")) { disk->type = VIR_DOMAIN_DISK_TYPE_NETWORK; disk->protocol = VIR_DOMAIN_DISK_PROTOCOL_RBD; @@ -8639,26 +8675,12 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr qemuCaps, goto no_memory; if (disk->type == VIR_DOMAIN_DISK_TYPE_NETWORK) { - char *host, *port; + char *port; switch (disk->protocol) { case VIR_DOMAIN_DISK_PROTOCOL_NBD: - host = disk->src; - port = strchr(host, ':'); - if (!port) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("cannot parse nbd filename '%s'"), disk->src); + if (qemuParseNBDString(disk) < 0) goto error; - } - *port++ = '\0'; - if (VIR_ALLOC(disk->hosts) < 0) - goto no_memory; - disk->nhosts = 1; - disk->hosts->name = host; - disk->hosts->port = strdup(port); - if (!disk->hosts->port) - goto no_memory; - disk->src = NULL; break; case VIR_DOMAIN_DISK_PROTOCOL_RBD: /* old-style CEPH_ARGS env variable is parsed later */ diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index 3f36896..2fa93a9 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -166,6 +166,7 @@ mymain(void) DO_TEST("disk-drive-cache-v1-wt"); DO_TEST("disk-drive-cache-v1-wb"); DO_TEST("disk-drive-cache-v1-none"); + DO_TEST("disk-drive-network-nbd"); DO_TEST("disk-scsi-device"); DO_TEST("disk-scsi-vscsi"); DO_TEST("disk-scsi-virtio-scsi"); -- 1.8.1.2
On 2013年02月26日 01:44, Paolo Bonzini wrote:
Move the code to an external function, and structure it to prepare the addition of new features in the next few patches.
Signed-off-by: Paolo Bonzini<pbonzini@redhat.com> --- src/qemu/qemu_command.c | 128 ++++++++++++++++++++++++++++-------------------- tests/qemuxml2xmltest.c | 1 + 2 files changed, 76 insertions(+), 53 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index a3c5a4e..beb7cfe 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -2128,6 +2128,45 @@ error: }
static int +qemuParseNBDString(virDomainDiskDefPtr disk) +{ + virDomainDiskHostDefPtr h = NULL; + char *host, *port; + + if (VIR_ALLOC(h)< 0) + goto no_memory; + + host = disk->src + strlen("nbd:"); + port = strchr(host, ':'); + if (!port) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("cannot parse nbd filename '%s'"), disk->src); + goto error; + } + + *port++ = '\0'; + h->name = strdup(host); + if (!h->name)
Trivial, but we prefer: if (!(h->name = strdup(host)))
+ goto no_memory; + + h->port = strdup(port); + if (!h->port)
Likewise.
+ goto no_memory; + + VIR_FREE(disk->src);
You lost setting defaults when refactoring: - def->hosts->transport = VIR_DOMAIN_DISK_PROTO_TRANS_TCP; - def->hosts->socket = NULL;
+ disk->nhosts = 1; + disk->hosts = h; + return 0; + +no_memory: + virReportOOMError(); +error: + virDomainDiskHostDefFree(h); + VIR_FREE(h); + return -1; +} + +static int qemuBuildGlusterString(virDomainDiskDefPtr disk, virBufferPtr opt) { int ret = -1; @@ -2188,6 +2227,36 @@ no_memory: goto cleanup; }
+static int +qemuBuildNBDString(virDomainDiskDefPtr disk, virBufferPtr opt) +{ + const char *transp; + + if (disk->nhosts != 1) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("nbd accepts only one host")); + return -1; + } + + virBufferAddLit(opt, "file=nbd:"); + + switch (disk->hosts->transport) { + case VIR_DOMAIN_DISK_PROTO_TRANS_TCP: + if (disk->hosts->name) + virBufferEscape(opt, ',', ",", "%s", disk->hosts->name); + virBufferEscape(opt, ',', ",", ":%s", + disk->hosts->port ? disk->hosts->port : "10809");
What's the magic (10809)? It's not in the old code, I guess it's the default port, but should we have a macro for it instead?
+ break; + default: + transp = virDomainDiskProtocolTransportTypeToString(disk->hosts->transport); + virReportError(VIR_ERR_INTERNAL_ERROR, + _("nbd does not support transport '%s'"), transp); + break; + } + + return 0; +} + char * qemuBuildDriveStr(virConnectPtr conn ATTRIBUTE_UNUSED, virDomainDiskDefPtr disk, @@ -2314,13 +2383,9 @@ qemuBuildDriveStr(virConnectPtr conn ATTRIBUTE_UNUSED, } else if (disk->type == VIR_DOMAIN_DISK_TYPE_NETWORK) { switch (disk->protocol) { case VIR_DOMAIN_DISK_PROTOCOL_NBD: - if (disk->nhosts != 1) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("NBD accepts only one host")); + if (qemuBuildNBDString(disk,&opt)< 0) goto error; - } - virBufferAsprintf(&opt, "file=nbd:%s:%s,", - disk->hosts->name, disk->hosts->port); + virBufferAddChar(&opt, ','); break; case VIR_DOMAIN_DISK_PROTOCOL_RBD: virBufferAddLit(&opt, "file="); @@ -7337,39 +7402,11 @@ qemuParseCommandLineDisk(virCapsPtr qemuCaps, if (STRPREFIX(def->src, "/dev/")) def->type = VIR_DOMAIN_DISK_TYPE_BLOCK; else if (STRPREFIX(def->src, "nbd:")) { - char *host, *port; - def->type = VIR_DOMAIN_DISK_TYPE_NETWORK; def->protocol = VIR_DOMAIN_DISK_PROTOCOL_NBD; - host = def->src + strlen("nbd:"); - port = strchr(host, ':'); - if (!port) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("cannot parse nbd filename '%s'"), - def->src); - goto error; - } - *port++ = '\0'; - if (VIR_ALLOC(def->hosts)< 0) { - virReportOOMError(); - goto error; - } - def->nhosts = 1; - def->hosts->name = strdup(host); - if (!def->hosts->name) { - virReportOOMError(); - goto error; - } - def->hosts->port = strdup(port); - if (!def->hosts->port) { - virReportOOMError(); - goto error; - } - def->hosts->transport = VIR_DOMAIN_DISK_PROTO_TRANS_TCP; - def->hosts->socket = NULL;
- VIR_FREE(def->src); - def->src = NULL; + if (qemuParseNBDString(def)< 0) + goto error; } else if (STRPREFIX(def->src, "rbd:")) { char *p = def->src;
@@ -8599,7 +8636,6 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr qemuCaps, else if (STRPREFIX(val, "nbd:")) { disk->type = VIR_DOMAIN_DISK_TYPE_NETWORK; disk->protocol = VIR_DOMAIN_DISK_PROTOCOL_NBD; - val += strlen("nbd:"); } else if (STRPREFIX(val, "rbd:")) { disk->type = VIR_DOMAIN_DISK_TYPE_NETWORK; disk->protocol = VIR_DOMAIN_DISK_PROTOCOL_RBD; @@ -8639,26 +8675,12 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr qemuCaps, goto no_memory;
if (disk->type == VIR_DOMAIN_DISK_TYPE_NETWORK) { - char *host, *port; + char *port;
switch (disk->protocol) { case VIR_DOMAIN_DISK_PROTOCOL_NBD: - host = disk->src; - port = strchr(host, ':'); - if (!port) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("cannot parse nbd filename '%s'"), disk->src); + if (qemuParseNBDString(disk)< 0) goto error; - } - *port++ = '\0'; - if (VIR_ALLOC(disk->hosts)< 0) - goto no_memory; - disk->nhosts = 1; - disk->hosts->name = host; - disk->hosts->port = strdup(port); - if (!disk->hosts->port) - goto no_memory; - disk->src = NULL; break; case VIR_DOMAIN_DISK_PROTOCOL_RBD: /* old-style CEPH_ARGS env variable is parsed later */ diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index 3f36896..2fa93a9 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -166,6 +166,7 @@ mymain(void) DO_TEST("disk-drive-cache-v1-wt"); DO_TEST("disk-drive-cache-v1-wb"); DO_TEST("disk-drive-cache-v1-none"); + DO_TEST("disk-drive-network-nbd"); DO_TEST("disk-scsi-device"); DO_TEST("disk-scsi-vscsi"); DO_TEST("disk-scsi-virtio-scsi");
Il 11/03/2013 04:38, Osier Yang ha scritto:
On 2013年02月26日 01:44, Paolo Bonzini wrote:
Move the code to an external function, and structure it to prepare the addition of new features in the next few patches.
Signed-off-by: Paolo Bonzini<pbonzini@redhat.com> --- src/qemu/qemu_command.c | 128 ++++++++++++++++++++++++++++-------------------- tests/qemuxml2xmltest.c | 1 + 2 files changed, 76 insertions(+), 53 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index a3c5a4e..beb7cfe 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -2128,6 +2128,45 @@ error: }
static int +qemuParseNBDString(virDomainDiskDefPtr disk) +{ + virDomainDiskHostDefPtr h = NULL; + char *host, *port; + + if (VIR_ALLOC(h)< 0) + goto no_memory; + + host = disk->src + strlen("nbd:"); + port = strchr(host, ':'); + if (!port) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("cannot parse nbd filename '%s'"), disk->src); + goto error; + } + + *port++ = '\0'; + h->name = strdup(host); + if (!h->name)
Trivial, but we prefer:
if (!(h->name = strdup(host)))
+ goto no_memory; + + h->port = strdup(port); + if (!h->port)
Likewise.
Ok.
+ goto no_memory; + + VIR_FREE(disk->src);
You lost setting defaults when refactoring:
- def->hosts->transport = VIR_DOMAIN_DISK_PROTO_TRANS_TCP; - def->hosts->socket = NULL;
These are both zero in memory, and VIR_ALLOC guarantees that.
+ disk->nhosts = 1; + disk->hosts = h; + return 0; + +no_memory: + virReportOOMError(); +error: + virDomainDiskHostDefFree(h); + VIR_FREE(h); + return -1; +} + +static int qemuBuildGlusterString(virDomainDiskDefPtr disk, virBufferPtr opt) { int ret = -1; @@ -2188,6 +2227,36 @@ no_memory: goto cleanup; }
+static int +qemuBuildNBDString(virDomainDiskDefPtr disk, virBufferPtr opt) +{ + const char *transp; + + if (disk->nhosts != 1) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("nbd accepts only one host")); + return -1; + } + + virBufferAddLit(opt, "file=nbd:"); + + switch (disk->hosts->transport) { + case VIR_DOMAIN_DISK_PROTO_TRANS_TCP: + if (disk->hosts->name) + virBufferEscape(opt, ',', ",", "%s", disk->hosts->name); + virBufferEscape(opt, ',', ",", ":%s", + disk->hosts->port ? disk->hosts->port : "10809");
What's the magic (10809)? It's not in the old code, I guess it's the default port, but should we have a macro for it instead?
Your choice. Paolo
+ break; + default: + transp = virDomainDiskProtocolTransportTypeToString(disk->hosts->transport); + virReportError(VIR_ERR_INTERNAL_ERROR, + _("nbd does not support transport '%s'"), transp); + break; + } + + return 0; +} + char * qemuBuildDriveStr(virConnectPtr conn ATTRIBUTE_UNUSED, virDomainDiskDefPtr disk, @@ -2314,13 +2383,9 @@ qemuBuildDriveStr(virConnectPtr conn ATTRIBUTE_UNUSED, } else if (disk->type == VIR_DOMAIN_DISK_TYPE_NETWORK) { switch (disk->protocol) { case VIR_DOMAIN_DISK_PROTOCOL_NBD: - if (disk->nhosts != 1) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("NBD accepts only one host")); + if (qemuBuildNBDString(disk,&opt)< 0) goto error; - } - virBufferAsprintf(&opt, "file=nbd:%s:%s,", - disk->hosts->name, disk->hosts->port); + virBufferAddChar(&opt, ','); break; case VIR_DOMAIN_DISK_PROTOCOL_RBD: virBufferAddLit(&opt, "file="); @@ -7337,39 +7402,11 @@ qemuParseCommandLineDisk(virCapsPtr qemuCaps, if (STRPREFIX(def->src, "/dev/")) def->type = VIR_DOMAIN_DISK_TYPE_BLOCK; else if (STRPREFIX(def->src, "nbd:")) { - char *host, *port; - def->type = VIR_DOMAIN_DISK_TYPE_NETWORK; def->protocol = VIR_DOMAIN_DISK_PROTOCOL_NBD; - host = def->src + strlen("nbd:"); - port = strchr(host, ':'); - if (!port) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("cannot parse nbd filename '%s'"), - def->src); - goto error; - } - *port++ = '\0'; - if (VIR_ALLOC(def->hosts)< 0) { - virReportOOMError(); - goto error; - } - def->nhosts = 1; - def->hosts->name = strdup(host); - if (!def->hosts->name) { - virReportOOMError(); - goto error; - } - def->hosts->port = strdup(port); - if (!def->hosts->port) { - virReportOOMError(); - goto error; - } - def->hosts->transport = VIR_DOMAIN_DISK_PROTO_TRANS_TCP; - def->hosts->socket = NULL;
- VIR_FREE(def->src); - def->src = NULL; + if (qemuParseNBDString(def)< 0) + goto error; } else if (STRPREFIX(def->src, "rbd:")) { char *p = def->src;
@@ -8599,7 +8636,6 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr qemuCaps, else if (STRPREFIX(val, "nbd:")) { disk->type = VIR_DOMAIN_DISK_TYPE_NETWORK; disk->protocol = VIR_DOMAIN_DISK_PROTOCOL_NBD; - val += strlen("nbd:"); } else if (STRPREFIX(val, "rbd:")) { disk->type = VIR_DOMAIN_DISK_TYPE_NETWORK; disk->protocol = VIR_DOMAIN_DISK_PROTOCOL_RBD; @@ -8639,26 +8675,12 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr qemuCaps, goto no_memory;
if (disk->type == VIR_DOMAIN_DISK_TYPE_NETWORK) { - char *host, *port; + char *port;
switch (disk->protocol) { case VIR_DOMAIN_DISK_PROTOCOL_NBD: - host = disk->src; - port = strchr(host, ':'); - if (!port) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("cannot parse nbd filename '%s'"), disk->src); + if (qemuParseNBDString(disk)< 0) goto error; - } - *port++ = '\0'; - if (VIR_ALLOC(disk->hosts)< 0) - goto no_memory; - disk->nhosts = 1; - disk->hosts->name = host; - disk->hosts->port = strdup(port); - if (!disk->hosts->port) - goto no_memory; - disk->src = NULL; break; case VIR_DOMAIN_DISK_PROTOCOL_RBD: /* old-style CEPH_ARGS env variable is parsed later */ diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index 3f36896..2fa93a9 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -166,6 +166,7 @@ mymain(void) DO_TEST("disk-drive-cache-v1-wt"); DO_TEST("disk-drive-cache-v1-wb"); DO_TEST("disk-drive-cache-v1-none"); + DO_TEST("disk-drive-network-nbd"); DO_TEST("disk-scsi-device"); DO_TEST("disk-scsi-vscsi"); DO_TEST("disk-scsi-virtio-scsi");
On 03/11/2013 07:18 AM, Paolo Bonzini wrote:
+ h->name = strdup(host); + if (!h->name)
Trivial, but we prefer:
if (!(h->name = strdup(host)))
Personally, I don't care enough to rewrite it - adding parenthesis to save a line isn't documented in HACKING as a requirement.
+ virBufferEscape(opt, ',', ",", "%s", disk->hosts->name); + virBufferEscape(opt, ',', ",", ":%s", + disk->hosts->port ? disk->hosts->port : "10809");
What's the magic (10809)? It's not in the old code, I guess it's the default port, but should we have a macro for it instead?
Your choice.
A macro seems nice, so I added one: diff --git i/src/qemu/qemu_command.c w/src/qemu/qemu_command.c index 7232906..9e2d6a9 100644 --- i/src/qemu/qemu_command.c +++ w/src/qemu/qemu_command.c @@ -2473,6 +2473,8 @@ no_memory: goto cleanup; } +#define QEMU_DEFAULT_NBD_PORT "10809" + static int qemuBuildNBDString(virDomainDiskDefPtr disk, virBufferPtr opt) { @@ -2491,7 +2493,8 @@ qemuBuildNBDString(virDomainDiskDefPtr disk, virBufferPtr opt) if (disk->hosts->name) virBufferEscape(opt, ',', ",", "%s", disk->hosts->name); virBufferEscape(opt, ',', ",", ":%s", - disk->hosts->port ? disk->hosts->port : "10809"); + disk->hosts->port ? disk->hosts->port : + QEMU_DEFAULT_NBD_PORT); break; default: transp = virDomainDiskProtocolTransportTypeToString(disk->hosts->transport); On 03/15/2013 08:32 AM, Daniel P. Berrange wrote:
I would have had the 'parse' method further down near the other parse function which calls it, but no big deal.
I didn't bother with this.
+++ b/tests/qemuxml2xmltest.c @@ -166,6 +166,7 @@ mymain(void) DO_TEST("disk-drive-cache-v1-wt"); DO_TEST("disk-drive-cache-v1-wb"); DO_TEST("disk-drive-cache-v1-none"); + DO_TEST("disk-drive-network-nbd"); DO_TEST("disk-scsi-device"); DO_TEST("disk-scsi-vscsi"); DO_TEST("disk-scsi-virtio-scsi");
This test case addition could be pushed indepedently of the rest of this patch.
I did just that, with this commit message: commit b96e0c18e161d80c88af354ef952fe2b6013f20f Author: Paolo Bonzini <pbonzini@redhat.com> Date: Mon Feb 25 18:44:22 2013 +0100 qemu: test NBD command-line builder and parser Enable more testing of NBD parsing, to ensure rewrites work. Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> Signed-off-by: Eric Blake <eblake@redhat.com>
ACK
and pushed, as two patches. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Mon, Feb 25, 2013 at 06:44:22PM +0100, Paolo Bonzini wrote:
Move the code to an external function, and structure it to prepare the addition of new features in the next few patches.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- src/qemu/qemu_command.c | 128 ++++++++++++++++++++++++++++-------------------- tests/qemuxml2xmltest.c | 1 + 2 files changed, 76 insertions(+), 53 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index a3c5a4e..beb7cfe 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -2128,6 +2128,45 @@ error: }
static int +qemuParseNBDString(virDomainDiskDefPtr disk) +{ + virDomainDiskHostDefPtr h = NULL; + char *host, *port; + + if (VIR_ALLOC(h) < 0) + goto no_memory; + + host = disk->src + strlen("nbd:"); + port = strchr(host, ':'); + if (!port) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("cannot parse nbd filename '%s'"), disk->src); + goto error; + } + + *port++ = '\0'; + h->name = strdup(host); + if (!h->name) + goto no_memory; + + h->port = strdup(port); + if (!h->port) + goto no_memory; + + VIR_FREE(disk->src); + disk->nhosts = 1; + disk->hosts = h; + return 0; + +no_memory: + virReportOOMError(); +error: + virDomainDiskHostDefFree(h); + VIR_FREE(h); + return -1; +} +
I would have had the 'parse' method further down near the other parse function which calls it, but no big deal.
+static int qemuBuildGlusterString(virDomainDiskDefPtr disk, virBufferPtr opt) { int ret = -1; @@ -2188,6 +2227,36 @@ no_memory: goto cleanup; }
+static int +qemuBuildNBDString(virDomainDiskDefPtr disk, virBufferPtr opt) +{ + const char *transp; + + if (disk->nhosts != 1) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("nbd accepts only one host")); + return -1; + } + + virBufferAddLit(opt, "file=nbd:"); + + switch (disk->hosts->transport) { + case VIR_DOMAIN_DISK_PROTO_TRANS_TCP: + if (disk->hosts->name) + virBufferEscape(opt, ',', ",", "%s", disk->hosts->name); + virBufferEscape(opt, ',', ",", ":%s", + disk->hosts->port ? disk->hosts->port : "10809"); + break; + default: + transp = virDomainDiskProtocolTransportTypeToString(disk->hosts->transport); + virReportError(VIR_ERR_INTERNAL_ERROR, + _("nbd does not support transport '%s'"), transp); + break; + } + + return 0; +} + char * qemuBuildDriveStr(virConnectPtr conn ATTRIBUTE_UNUSED, virDomainDiskDefPtr disk, @@ -2314,13 +2383,9 @@ qemuBuildDriveStr(virConnectPtr conn ATTRIBUTE_UNUSED, } else if (disk->type == VIR_DOMAIN_DISK_TYPE_NETWORK) { switch (disk->protocol) { case VIR_DOMAIN_DISK_PROTOCOL_NBD: - if (disk->nhosts != 1) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("NBD accepts only one host")); + if (qemuBuildNBDString(disk, &opt) < 0) goto error; - } - virBufferAsprintf(&opt, "file=nbd:%s:%s,", - disk->hosts->name, disk->hosts->port); + virBufferAddChar(&opt, ','); break; case VIR_DOMAIN_DISK_PROTOCOL_RBD: virBufferAddLit(&opt, "file="); @@ -7337,39 +7402,11 @@ qemuParseCommandLineDisk(virCapsPtr qemuCaps, if (STRPREFIX(def->src, "/dev/")) def->type = VIR_DOMAIN_DISK_TYPE_BLOCK; else if (STRPREFIX(def->src, "nbd:")) { - char *host, *port; - def->type = VIR_DOMAIN_DISK_TYPE_NETWORK; def->protocol = VIR_DOMAIN_DISK_PROTOCOL_NBD; - host = def->src + strlen("nbd:"); - port = strchr(host, ':'); - if (!port) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("cannot parse nbd filename '%s'"), - def->src); - goto error; - } - *port++ = '\0'; - if (VIR_ALLOC(def->hosts) < 0) { - virReportOOMError(); - goto error; - } - def->nhosts = 1; - def->hosts->name = strdup(host); - if (!def->hosts->name) { - virReportOOMError(); - goto error; - } - def->hosts->port = strdup(port); - if (!def->hosts->port) { - virReportOOMError(); - goto error; - } - def->hosts->transport = VIR_DOMAIN_DISK_PROTO_TRANS_TCP; - def->hosts->socket = NULL;
- VIR_FREE(def->src); - def->src = NULL; + if (qemuParseNBDString(def) < 0) + goto error; } else if (STRPREFIX(def->src, "rbd:")) { char *p = def->src;
@@ -8599,7 +8636,6 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr qemuCaps, else if (STRPREFIX(val, "nbd:")) { disk->type = VIR_DOMAIN_DISK_TYPE_NETWORK; disk->protocol = VIR_DOMAIN_DISK_PROTOCOL_NBD; - val += strlen("nbd:"); } else if (STRPREFIX(val, "rbd:")) { disk->type = VIR_DOMAIN_DISK_TYPE_NETWORK; disk->protocol = VIR_DOMAIN_DISK_PROTOCOL_RBD; @@ -8639,26 +8675,12 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr qemuCaps, goto no_memory;
if (disk->type == VIR_DOMAIN_DISK_TYPE_NETWORK) { - char *host, *port; + char *port;
switch (disk->protocol) { case VIR_DOMAIN_DISK_PROTOCOL_NBD: - host = disk->src; - port = strchr(host, ':'); - if (!port) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("cannot parse nbd filename '%s'"), disk->src); + if (qemuParseNBDString(disk) < 0) goto error; - } - *port++ = '\0'; - if (VIR_ALLOC(disk->hosts) < 0) - goto no_memory; - disk->nhosts = 1; - disk->hosts->name = host; - disk->hosts->port = strdup(port); - if (!disk->hosts->port) - goto no_memory; - disk->src = NULL; break; case VIR_DOMAIN_DISK_PROTOCOL_RBD: /* old-style CEPH_ARGS env variable is parsed later */ diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index 3f36896..2fa93a9 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -166,6 +166,7 @@ mymain(void) DO_TEST("disk-drive-cache-v1-wt"); DO_TEST("disk-drive-cache-v1-wb"); DO_TEST("disk-drive-cache-v1-none"); + DO_TEST("disk-drive-network-nbd"); DO_TEST("disk-scsi-device"); DO_TEST("disk-scsi-vscsi"); DO_TEST("disk-scsi-virtio-scsi");
This test case addition could be pushed indepedently of the rest of this patch. ACK Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
These are supported by nbd-server and by the NBD server that QEMU embeds for live image access. Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- docs/formatdomain.html.in | 6 ++-- src/qemu/qemu_command.c | 17 +++++++++++ tests/qemuargv2xmltest.c | 1 + ...qemuxml2argv-disk-drive-network-nbd-export.args | 5 ++++ .../qemuxml2argv-disk-drive-network-nbd-export.xml | 33 ++++++++++++++++++++++ tests/qemuxml2argvtest.c | 2 ++ tests/qemuxml2xmltest.c | 1 + 7 files changed, 62 insertions(+), 3 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-nbd-export.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-nbd-export.xml diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index a9003d7..90cfc03 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -1441,9 +1441,9 @@ are "nbd", "rbd", "sheepdog" or "gluster". If the <code>protocol</code> attribute is "rbd", "sheepdog" or "gluster", an additional attribute <code>name</code> is mandatory to specify which - volume/image will be used. When the disk <code>type</code> is - "network", the <code>source</code> may have zero or - more <code>host</code> sub-elements used to specify the hosts + volume/image will be used; for "nbd" it is optional. When the disk + <code>type</code> is "network", the <code>source</code> may have zero + or more <code>host</code> sub-elements used to specify the hosts to connect. <span class="since">Since 0.0.3; <code>type='dir'</code> since 0.7.5; <code>type='network'</code> since 0.8.7</span><br/> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index beb7cfe..4762f0a 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -2132,6 +2132,7 @@ qemuParseNBDString(virDomainDiskDefPtr disk) { virDomainDiskHostDefPtr h = NULL; char *host, *port; + char *src; if (VIR_ALLOC(h) < 0) goto no_memory; @@ -2149,11 +2150,24 @@ qemuParseNBDString(virDomainDiskDefPtr disk) if (!h->name) goto no_memory; + src = strchr(port, ':'); + if (src) + *src++ = '\0'; + h->port = strdup(port); if (!h->port) goto no_memory; + if (src && STRPREFIX(src, "exportname=")) { + src = strdup(strchr(src, '=') + 1); + if (!src) + goto no_memory; + } else { + src = NULL; + } + VIR_FREE(disk->src); + disk->src = src; disk->nhosts = 1; disk->hosts = h; return 0; @@ -2254,6 +2268,9 @@ qemuBuildNBDString(virDomainDiskDefPtr disk, virBufferPtr opt) break; } + if (disk->src) + virBufferEscape(opt, ',', ",", ":exportname=%s", disk->src); + return 0; } diff --git a/tests/qemuargv2xmltest.c b/tests/qemuargv2xmltest.c index 3c23010..1731022 100644 --- a/tests/qemuargv2xmltest.c +++ b/tests/qemuargv2xmltest.c @@ -182,6 +182,7 @@ mymain(void) DO_TEST("disk-drive-cache-directsync"); DO_TEST("disk-drive-cache-unsafe"); DO_TEST("disk-drive-network-nbd"); + DO_TEST("disk-drive-network-nbd-export"); DO_TEST("disk-drive-network-gluster"); DO_TEST("disk-drive-network-rbd"); DO_TEST("disk-drive-network-rbd-ipv6"); diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-nbd-export.args b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-nbd-export.args new file mode 100644 index 0000000..bf411ff --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-nbd-export.args @@ -0,0 +1,5 @@ +LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test /usr/bin/qemu -S -M \ +pc -m 214 -smp 1 -nographic -monitor unix:/tmp/test-monitor,server,nowait \ +-no-acpi -boot c -usb -drive file=/dev/HostVG/QEMUGuest1,if=ide,bus=0,unit=0 -drive \ +file=nbd:example.org:6000:exportname=bar,if=virtio,format=raw -net none -serial none \ +-parallel none diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-nbd-export.xml b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-nbd-export.xml new file mode 100644 index 0000000..f2b5ca4 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-nbd-export.xml @@ -0,0 +1,33 @@ +<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</emulator> + <disk type='block' device='disk'> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='hda' bus='ide'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> + <disk type='network' device='disk'> + <driver name='qemu' type='raw'/> + <source protocol='nbd' name='bar'> + <host name='example.org' port='6000'/> + </source> + <target dev='vda' bus='virtio'/> + </disk> + <controller type='usb' index='0'/> + <controller type='ide' index='0'/> + <memballoon model='virtio'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 4357068..7d44be2 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -485,6 +485,8 @@ mymain(void) QEMU_CAPS_DRIVE_CACHE_UNSAFE, QEMU_CAPS_DRIVE_FORMAT); DO_TEST("disk-drive-network-nbd", QEMU_CAPS_DRIVE, QEMU_CAPS_DRIVE_FORMAT); + DO_TEST("disk-drive-network-nbd-export", + QEMU_CAPS_DRIVE, QEMU_CAPS_DRIVE_FORMAT); DO_TEST("disk-drive-network-gluster", QEMU_CAPS_DRIVE, QEMU_CAPS_DRIVE_FORMAT); DO_TEST("disk-drive-network-rbd", diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index 2fa93a9..cdf6554 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -167,6 +167,7 @@ mymain(void) DO_TEST("disk-drive-cache-v1-wb"); DO_TEST("disk-drive-cache-v1-none"); DO_TEST("disk-drive-network-nbd"); + DO_TEST("disk-drive-network-nbd-export"); DO_TEST("disk-scsi-device"); DO_TEST("disk-scsi-vscsi"); DO_TEST("disk-scsi-virtio-scsi"); -- 1.8.1.2
On 2013年02月26日 01:44, Paolo Bonzini wrote:
These are supported by nbd-server and by the NBD server that QEMU embeds for live image access.
Signed-off-by: Paolo Bonzini<pbonzini@redhat.com> --- docs/formatdomain.html.in | 6 ++-- src/qemu/qemu_command.c | 17 +++++++++++ tests/qemuargv2xmltest.c | 1 + ...qemuxml2argv-disk-drive-network-nbd-export.args | 5 ++++ .../qemuxml2argv-disk-drive-network-nbd-export.xml | 33 ++++++++++++++++++++++ tests/qemuxml2argvtest.c | 2 ++ tests/qemuxml2xmltest.c | 1 + 7 files changed, 62 insertions(+), 3 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-nbd-export.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-nbd-export.xml
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index a9003d7..90cfc03 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -1441,9 +1441,9 @@ are "nbd", "rbd", "sheepdog" or "gluster". If the <code>protocol</code> attribute is "rbd", "sheepdog" or "gluster", an additional attribute<code>name</code> is mandatory to specify which - volume/image will be used. When the disk<code>type</code> is - "network", the<code>source</code> may have zero or - more<code>host</code> sub-elements used to specify the hosts + volume/image will be used; for "nbd" it is optional. When the disk +<code>type</code> is "network", the<code>source</code> may have zero + or more<code>host</code> sub-elements used to specify the hosts to connect. <span class="since">Since 0.0.3;<code>type='dir'</code> since 0.7.5;<code>type='network'</code> since 0.8.7</span><br/> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index beb7cfe..4762f0a 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -2132,6 +2132,7 @@ qemuParseNBDString(virDomainDiskDefPtr disk) { virDomainDiskHostDefPtr h = NULL; char *host, *port; + char *src;
if (VIR_ALLOC(h)< 0) goto no_memory; @@ -2149,11 +2150,24 @@ qemuParseNBDString(virDomainDiskDefPtr disk) if (!h->name) goto no_memory;
+ src = strchr(port, ':'); + if (src)
if (!(src = strchr(port, ':')))
+ *src++ = '\0'; + h->port = strdup(port); if (!h->port) goto no_memory;
+ if (src&& STRPREFIX(src, "exportname=")) { + src = strdup(strchr(src, '=') + 1); + if (!src)
Likewise.
+ goto no_memory; + } else { + src = NULL; + } + VIR_FREE(disk->src); + disk->src = src; disk->nhosts = 1; disk->hosts = h; return 0; @@ -2254,6 +2268,9 @@ qemuBuildNBDString(virDomainDiskDefPtr disk, virBufferPtr opt) break; }
+ if (disk->src) + virBufferEscape(opt, ',', ",", ":exportname=%s", disk->src); + return 0; }
diff --git a/tests/qemuargv2xmltest.c b/tests/qemuargv2xmltest.c index 3c23010..1731022 100644 --- a/tests/qemuargv2xmltest.c +++ b/tests/qemuargv2xmltest.c @@ -182,6 +182,7 @@ mymain(void) DO_TEST("disk-drive-cache-directsync"); DO_TEST("disk-drive-cache-unsafe"); DO_TEST("disk-drive-network-nbd"); + DO_TEST("disk-drive-network-nbd-export"); DO_TEST("disk-drive-network-gluster"); DO_TEST("disk-drive-network-rbd"); DO_TEST("disk-drive-network-rbd-ipv6"); diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-nbd-export.args b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-nbd-export.args new file mode 100644 index 0000000..bf411ff --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-nbd-export.args @@ -0,0 +1,5 @@ +LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test /usr/bin/qemu -S -M \ +pc -m 214 -smp 1 -nographic -monitor unix:/tmp/test-monitor,server,nowait \ +-no-acpi -boot c -usb -drive file=/dev/HostVG/QEMUGuest1,if=ide,bus=0,unit=0 -drive \ +file=nbd:example.org:6000:exportname=bar,if=virtio,format=raw -net none -serial none \ +-parallel none diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-nbd-export.xml b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-nbd-export.xml new file mode 100644 index 0000000..f2b5ca4 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-nbd-export.xml @@ -0,0 +1,33 @@ +<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</emulator> +<disk type='block' device='disk'> +<source dev='/dev/HostVG/QEMUGuest1'/> +<target dev='hda' bus='ide'/> +<address type='drive' controller='0' bus='0' target='0' unit='0'/> +</disk> +<disk type='network' device='disk'> +<driver name='qemu' type='raw'/> +<source protocol='nbd' name='bar'> +<host name='example.org' port='6000'/> +</source> +<target dev='vda' bus='virtio'/> +</disk> +<controller type='usb' index='0'/> +<controller type='ide' index='0'/> +<memballoon model='virtio'/> +</devices> +</domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 4357068..7d44be2 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -485,6 +485,8 @@ mymain(void) QEMU_CAPS_DRIVE_CACHE_UNSAFE, QEMU_CAPS_DRIVE_FORMAT); DO_TEST("disk-drive-network-nbd", QEMU_CAPS_DRIVE, QEMU_CAPS_DRIVE_FORMAT); + DO_TEST("disk-drive-network-nbd-export", + QEMU_CAPS_DRIVE, QEMU_CAPS_DRIVE_FORMAT); DO_TEST("disk-drive-network-gluster", QEMU_CAPS_DRIVE, QEMU_CAPS_DRIVE_FORMAT); DO_TEST("disk-drive-network-rbd", diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index 2fa93a9..cdf6554 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -167,6 +167,7 @@ mymain(void) DO_TEST("disk-drive-cache-v1-wb"); DO_TEST("disk-drive-cache-v1-none"); DO_TEST("disk-drive-network-nbd"); + DO_TEST("disk-drive-network-nbd-export"); DO_TEST("disk-scsi-device"); DO_TEST("disk-scsi-vscsi"); DO_TEST("disk-scsi-virtio-scsi");
ACK with the coding style changed to be consistent.
On 03/10/2013 09:45 PM, Osier Yang wrote:
On 2013年02月26日 01:44, Paolo Bonzini wrote:
These are supported by nbd-server and by the NBD server that QEMU embeds for live image access.
Signed-off-by: Paolo Bonzini<pbonzini@redhat.com> ---
+++ b/src/qemu/qemu_command.c @@ -2132,6 +2132,7 @@ qemuParseNBDString(virDomainDiskDefPtr disk) { virDomainDiskHostDefPtr h = NULL; char *host, *port; + char *src;
if (VIR_ALLOC(h)< 0) goto no_memory; @@ -2149,11 +2150,24 @@ qemuParseNBDString(virDomainDiskDefPtr disk) if (!h->name) goto no_memory;
+ src = strchr(port, ':'); + if (src)
if (!(src = strchr(port, ':')))
Not worth compressing, since our HACKING doesn't mandate it.
ACK with the coding style changed to be consistent.
I did make one tweak:
+++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-nbd-export.args @@ -0,0 +1,5 @@ +LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test /usr/bin/qemu -S -M \ +pc -m 214 -smp 1 -nographic -monitor unix:/tmp/test-monitor,server,nowait \ +-no-acpi -boot c -usb -drive file=/dev/HostVG/QEMUGuest1,if=ide,bus=0,unit=0 -drive \ +file=nbd:example.org:6000:exportname=bar,if=virtio,format=raw -net none -serial none \ +-parallel none
These lines were longer than 80 columns, so I redid where the backslash-newline pairs lived. Now pushed. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Mon, Feb 25, 2013 at 06:44:23PM +0100, Paolo Bonzini wrote:
These are supported by nbd-server and by the NBD server that QEMU embeds for live image access.
But seemingly not by 'qemu-nbd' ? ACK Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
Il 15/03/2013 15:34, Daniel P. Berrange ha scritto:
These are supported by nbd-server and by the NBD server that QEMU embeds for live image access. But seemingly not by 'qemu-nbd' ?
No, not yet at least. Paolo
This reuses the XML format that was introduced for Gluster. Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- docs/formatdomain.html.in | 8 ++-- src/qemu/qemu_command.c | 49 +++++++++++++++------- tests/qemuargv2xmltest.c | 1 + .../qemuxml2argv-disk-drive-network-nbd-unix.args | 5 +++ .../qemuxml2argv-disk-drive-network-nbd-unix.xml | 33 +++++++++++++++ tests/qemuxml2argvtest.c | 2 + tests/qemuxml2xmltest.c | 1 + 7 files changed, 80 insertions(+), 19 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-nbd-unix.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-nbd-unix.xml diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 90cfc03..094b509 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -1713,9 +1713,11 @@ <td> only one </td> </tr> </table> - In case of gluster, valid values for transport attribute are tcp, rdma - or unix. If nothing is specified, tcp is assumed. If transport is unix, - socket attribute specifies path to unix socket. + gluster supports "tcp", "rdma", "unix" as valid values for the + transport attribute. nbd supports "tcp" and "unix". Others only + support "tcp". If nothing is specified, "tcp" is assumed. If the + transport is "unix", the socket attribute specifies the path to an + AF_UNIX socket. </dd> <dt><code>address</code></dt> <dd>If present, the <code>address</code> element ties the disk diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 4762f0a..89cd065 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -2138,25 +2138,34 @@ qemuParseNBDString(virDomainDiskDefPtr disk) goto no_memory; host = disk->src + strlen("nbd:"); - port = strchr(host, ':'); - if (!port) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("cannot parse nbd filename '%s'"), disk->src); - goto error; - } + if (STRPREFIX(host, "unix:/")) { + src = strchr(host + strlen("unix:"), ':'); + if (src) + *src++ = '\0'; - *port++ = '\0'; - h->name = strdup(host); - if (!h->name) - goto no_memory; + h->transport = VIR_DOMAIN_DISK_PROTO_TRANS_UNIX; + h->socket = strdup(host + strlen("unix:")); + } else { + port = strchr(host, ':'); + if (!port) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("cannot parse nbd filename '%s'"), disk->src); + goto error; + } - src = strchr(port, ':'); - if (src) - *src++ = '\0'; + *port++ = '\0'; + h->name = strdup(host); + if (!h->name) + goto no_memory; - h->port = strdup(port); - if (!h->port) - goto no_memory; + src = strchr(port, ':'); + if (src) + *src++ = '\0'; + + h->port = strdup(port); + if (!h->port) + goto no_memory; + } if (src && STRPREFIX(src, "exportname=")) { src = strdup(strchr(src, '=') + 1); @@ -2261,6 +2270,14 @@ qemuBuildNBDString(virDomainDiskDefPtr disk, virBufferPtr opt) virBufferEscape(opt, ',', ",", ":%s", disk->hosts->port ? disk->hosts->port : "10809"); break; + case VIR_DOMAIN_DISK_PROTO_TRANS_UNIX: + if (!disk->hosts->socket) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("socket attribute required for unix transport")); + return -1; + } + virBufferEscape(opt, ',', ",", "unix:%s", disk->hosts->socket); + break; default: transp = virDomainDiskProtocolTransportTypeToString(disk->hosts->transport); virReportError(VIR_ERR_INTERNAL_ERROR, diff --git a/tests/qemuargv2xmltest.c b/tests/qemuargv2xmltest.c index 1731022..12174b3 100644 --- a/tests/qemuargv2xmltest.c +++ b/tests/qemuargv2xmltest.c @@ -183,6 +183,7 @@ mymain(void) DO_TEST("disk-drive-cache-unsafe"); DO_TEST("disk-drive-network-nbd"); DO_TEST("disk-drive-network-nbd-export"); + DO_TEST("disk-drive-network-nbd-unix"); DO_TEST("disk-drive-network-gluster"); DO_TEST("disk-drive-network-rbd"); DO_TEST("disk-drive-network-rbd-ipv6"); diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-nbd-unix.args b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-nbd-unix.args new file mode 100644 index 0000000..afea187 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-nbd-unix.args @@ -0,0 +1,5 @@ +LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test /usr/bin/qemu -S -M \ +pc -m 214 -smp 1 -nographic -monitor unix:/tmp/test-monitor,server,nowait \ +-no-acpi -boot c -usb -drive file=/dev/HostVG/QEMUGuest1,if=ide,bus=0,unit=0 -drive \ +file=nbd:unix:/var/run/nbdsock:exportname=bar,if=virtio,format=raw -net none -serial none \ +-parallel none diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-nbd-unix.xml b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-nbd-unix.xml new file mode 100644 index 0000000..46114d5 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-nbd-unix.xml @@ -0,0 +1,33 @@ +<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</emulator> + <disk type='block' device='disk'> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='hda' bus='ide'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> + <disk type='network' device='disk'> + <driver name='qemu' type='raw'/> + <source protocol='nbd' name='bar'> + <host transport='unix' socket='/var/run/nbdsock'/> + </source> + <target dev='vda' bus='virtio'/> + </disk> + <controller type='usb' index='0'/> + <controller type='ide' index='0'/> + <memballoon model='virtio'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 7d44be2..db09633 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -487,6 +487,8 @@ mymain(void) QEMU_CAPS_DRIVE, QEMU_CAPS_DRIVE_FORMAT); DO_TEST("disk-drive-network-nbd-export", QEMU_CAPS_DRIVE, QEMU_CAPS_DRIVE_FORMAT); + DO_TEST("disk-drive-network-nbd-unix", + QEMU_CAPS_DRIVE, QEMU_CAPS_DRIVE_FORMAT); DO_TEST("disk-drive-network-gluster", QEMU_CAPS_DRIVE, QEMU_CAPS_DRIVE_FORMAT); DO_TEST("disk-drive-network-rbd", diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index cdf6554..d698f59 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -168,6 +168,7 @@ mymain(void) DO_TEST("disk-drive-cache-v1-none"); DO_TEST("disk-drive-network-nbd"); DO_TEST("disk-drive-network-nbd-export"); + DO_TEST("disk-drive-network-nbd-unix"); DO_TEST("disk-scsi-device"); DO_TEST("disk-scsi-vscsi"); DO_TEST("disk-scsi-virtio-scsi"); -- 1.8.1.2
On 2013年02月26日 01:44, Paolo Bonzini wrote:
This reuses the XML format that was introduced for Gluster.
Signed-off-by: Paolo Bonzini<pbonzini@redhat.com> --- docs/formatdomain.html.in | 8 ++-- src/qemu/qemu_command.c | 49 +++++++++++++++------- tests/qemuargv2xmltest.c | 1 + .../qemuxml2argv-disk-drive-network-nbd-unix.args | 5 +++ .../qemuxml2argv-disk-drive-network-nbd-unix.xml | 33 +++++++++++++++ tests/qemuxml2argvtest.c | 2 + tests/qemuxml2xmltest.c | 1 + 7 files changed, 80 insertions(+), 19 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-nbd-unix.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-nbd-unix.xml
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 90cfc03..094b509 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -1713,9 +1713,11 @@ <td> only one</td> </tr> </table> - In case of gluster, valid values for transport attribute are tcp, rdma - or unix. If nothing is specified, tcp is assumed. If transport is unix, - socket attribute specifies path to unix socket. + gluster supports "tcp", "rdma", "unix" as valid values for the + transport attribute. nbd supports "tcp" and "unix". Others only
Generally we want "attribute <code>transport</code>", to make it more clear in generated html pages.
+ support "tcp". If nothing is specified, "tcp" is assumed. If the + transport is "unix", the socket attribute specifies the path to an + AF_UNIX socket.
Likewise, "attribute <code>socket</code>".
</dd> <dt><code>address</code></dt> <dd>If present, the<code>address</code> element ties the disk diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 4762f0a..89cd065 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -2138,25 +2138,34 @@ qemuParseNBDString(virDomainDiskDefPtr disk) goto no_memory;
host = disk->src + strlen("nbd:"); - port = strchr(host, ':'); - if (!port) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("cannot parse nbd filename '%s'"), disk->src); - goto error; - } + if (STRPREFIX(host, "unix:/")) { + src = strchr(host + strlen("unix:"), ':'); + if (src) + *src++ = '\0';
Same comments as previous patches.
- *port++ = '\0'; - h->name = strdup(host); - if (!h->name) - goto no_memory; + h->transport = VIR_DOMAIN_DISK_PROTO_TRANS_UNIX; + h->socket = strdup(host + strlen("unix:")); + } else { + port = strchr(host, ':'); + if (!port) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("cannot parse nbd filename '%s'"), disk->src); + goto error; + }
- src = strchr(port, ':'); - if (src) - *src++ = '\0'; + *port++ = '\0'; + h->name = strdup(host); + if (!h->name) + goto no_memory;
- h->port = strdup(port); - if (!h->port) - goto no_memory; + src = strchr(port, ':'); + if (src) + *src++ = '\0'; + + h->port = strdup(port); + if (!h->port) + goto no_memory; + }
if (src&& STRPREFIX(src, "exportname=")) { src = strdup(strchr(src, '=') + 1); @@ -2261,6 +2270,14 @@ qemuBuildNBDString(virDomainDiskDefPtr disk, virBufferPtr opt) virBufferEscape(opt, ',', ",", ":%s", disk->hosts->port ? disk->hosts->port : "10809"); break; + case VIR_DOMAIN_DISK_PROTO_TRANS_UNIX: + if (!disk->hosts->socket) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("socket attribute required for unix transport")); + return -1; + }
Not sure if we should do this when parsing, as I think the "socket" is required as long as the transport protocol is "unix", unless it's generated somewhere. Others look good. Osier
Il 11/03/2013 10:34, Osier Yang ha scritto:
+ if (STRPREFIX(host, "unix:/")) { + src = strchr(host + strlen("unix:"), ':'); + if (src) + *src++ = '\0';
Same comments as previous patches.
I think in this case the value that is assigned to src is complex enough that it's better to keep it separate.
- *port++ = '\0'; - h->name = strdup(host); - if (!h->name) - goto no_memory; + h->transport = VIR_DOMAIN_DISK_PROTO_TRANS_UNIX; + h->socket = strdup(host + strlen("unix:")); + } else { + port = strchr(host, ':'); + if (!port) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("cannot parse nbd filename '%s'"), disk->src); + goto error; + }
- src = strchr(port, ':'); - if (src) - *src++ = '\0'; + *port++ = '\0'; + h->name = strdup(host); + if (!h->name) + goto no_memory;
- h->port = strdup(port); - if (!h->port) - goto no_memory; + src = strchr(port, ':'); + if (src) + *src++ = '\0'; + + h->port = strdup(port); + if (!h->port) + goto no_memory; + }
if (src&& STRPREFIX(src, "exportname=")) { src = strdup(strchr(src, '=') + 1); @@ -2261,6 +2270,14 @@ qemuBuildNBDString(virDomainDiskDefPtr disk, virBufferPtr opt) virBufferEscape(opt, ',', ",", ":%s", disk->hosts->port ? disk->hosts->port : "10809"); break; + case VIR_DOMAIN_DISK_PROTO_TRANS_UNIX: + if (!disk->hosts->socket) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("socket attribute required for unix transport")); + return -1; + }
Not sure if we should do this when parsing, as I think the "socket" is required as long as the transport protocol is "unix", unless it's generated somewhere.
Yeah, we cannot be sure in general that it will be required for _all_ protocols, so I kept it here and followed what gluster does. Paolo
On 2013年03月11日 21:20, Paolo Bonzini wrote:
Il 11/03/2013 10:34, Osier Yang ha scritto:
+ if (STRPREFIX(host, "unix:/")) { + src = strchr(host + strlen("unix:"), ':'); + if (src) + *src++ = '\0';
Same comments as previous patches.
I think in this case the value that is assigned to src is complex enough that it's better to keep it separate.
Okay, but still applies to other similar codes.
- *port++ = '\0'; - h->name = strdup(host); - if (!h->name) - goto no_memory; + h->transport = VIR_DOMAIN_DISK_PROTO_TRANS_UNIX; + h->socket = strdup(host + strlen("unix:")); + } else { + port = strchr(host, ':'); + if (!port) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("cannot parse nbd filename '%s'"), disk->src); + goto error; + }
- src = strchr(port, ':'); - if (src) - *src++ = '\0'; + *port++ = '\0'; + h->name = strdup(host); + if (!h->name) + goto no_memory;
- h->port = strdup(port); - if (!h->port) - goto no_memory; + src = strchr(port, ':'); + if (src) + *src++ = '\0'; + + h->port = strdup(port); + if (!h->port) + goto no_memory; + }
if (src&& STRPREFIX(src, "exportname=")) { src = strdup(strchr(src, '=') + 1); @@ -2261,6 +2270,14 @@ qemuBuildNBDString(virDomainDiskDefPtr disk, virBufferPtr opt) virBufferEscape(opt, ',', ",", ":%s", disk->hosts->port ? disk->hosts->port : "10809"); break; + case VIR_DOMAIN_DISK_PROTO_TRANS_UNIX: + if (!disk->hosts->socket) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("socket attribute required for unix transport")); + return -1; + }
Not sure if we should do this when parsing, as I think the "socket" is required as long as the transport protocol is "unix", unless it's generated somewhere.
Yeah, we cannot be sure in general that it will be required for _all_ protocols, so I kept it here and followed what gluster does.
Agreed.
On Mon, Feb 25, 2013 at 06:44:24PM +0100, Paolo Bonzini wrote:
This reuses the XML format that was introduced for Gluster.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- docs/formatdomain.html.in | 8 ++-- src/qemu/qemu_command.c | 49 +++++++++++++++------- tests/qemuargv2xmltest.c | 1 + .../qemuxml2argv-disk-drive-network-nbd-unix.args | 5 +++ .../qemuxml2argv-disk-drive-network-nbd-unix.xml | 33 +++++++++++++++ tests/qemuxml2argvtest.c | 2 + tests/qemuxml2xmltest.c | 1 + 7 files changed, 80 insertions(+), 19 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-nbd-unix.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-nbd-unix.xml
ACK Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 03/15/2013 08:35 AM, Daniel P. Berrange wrote:
On Mon, Feb 25, 2013 at 06:44:24PM +0100, Paolo Bonzini wrote:
This reuses the XML format that was introduced for Gluster.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- docs/formatdomain.html.in | 8 ++-- src/qemu/qemu_command.c | 49 +++++++++++++++------- tests/qemuargv2xmltest.c | 1 + .../qemuxml2argv-disk-drive-network-nbd-unix.args | 5 +++ .../qemuxml2argv-disk-drive-network-nbd-unix.xml | 33 +++++++++++++++ tests/qemuxml2argvtest.c | 2 + tests/qemuxml2xmltest.c | 1 + 7 files changed, 80 insertions(+), 19 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-nbd-unix.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-nbd-unix.xml
ACK
Pushed, after fixing this 'make syntax-check' failure: src/qemu/qemu_command.c:2524: virReportError(VIR_ERR_INTERNAL_ERROR, src/qemu/qemu_command.c-2525- _("socket attribute required for unix transport")); maint.mk: found diagnostic without % and rewrapping tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-nbd-unix.args to fit 80 columns. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
QEMU 1.3 and newer support an alternative URI-based syntax to specify the location of an NBD server. Libvirt can keep on using the old syntax in general, but only the URI syntax supports IPv6 addresses. The URI syntax also supports relative paths to Unix sockets. These should never be used but aren't explicitly blocked either by the parser, so support it just in case. The URI syntax is intentionally compatible with Gluster's, and the code can be reused. Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- src/qemu/qemu_command.c | 97 +++++++++++++++------- tests/qemuargv2xmltest.c | 2 + ...ml2argv-disk-drive-network-nbd-ipv6-export.args | 5 ++ ...xml2argv-disk-drive-network-nbd-ipv6-export.xml | 33 ++++++++ .../qemuxml2argv-disk-drive-network-nbd-ipv6.args | 5 ++ .../qemuxml2argv-disk-drive-network-nbd-ipv6.xml | 33 ++++++++ tests/qemuxml2argvtest.c | 4 + tests/qemuxml2xmltest.c | 2 + 8 files changed, 153 insertions(+), 28 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-nbd-ipv6-export.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-nbd-ipv6-export.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-nbd-ipv6.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-nbd-ipv6.xml diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 89cd065..733d3bf 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -2050,35 +2050,36 @@ no_memory: } static int -qemuParseGlusterString(virDomainDiskDefPtr def) +qemuParseDriveURIString(virDomainDiskDefPtr def, virURIPtr uri, + const char *scheme) { int ret = -1; char *transp = NULL; char *sock = NULL; char *volimg = NULL; - virURIPtr uri = NULL; - - if (!(uri = virURIParse(def->src))) { - return -1; - } if (VIR_ALLOC(def->hosts) < 0) goto no_memory; - if (STREQ(uri->scheme, "gluster")) { + transp = strchr(uri->scheme, '+'); + if (transp) + *transp++ = 0; + + if (!STREQ(uri->scheme, scheme)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Invalid transport/scheme '%s'"), uri->scheme); + goto error; + } + + if (!transp) { def->hosts->transport = VIR_DOMAIN_DISK_PROTO_TRANS_TCP; - } else if (STRPREFIX(uri->scheme, "gluster+")) { - transp = strchr(uri->scheme, '+') + 1; + } else { def->hosts->transport = virDomainDiskProtocolTransportTypeFromString(transp); if (def->hosts->transport < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, - _("Invalid gluster transport type '%s'"), transp); + _("Invalid %s transport type '%s'"), scheme, transp); goto error; } - } else { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Invalid transport/scheme '%s'"), uri->scheme); - goto error; } def->nhosts = 0; /* set to 1 once everything succeeds */ @@ -2105,11 +2106,16 @@ qemuParseGlusterString(virDomainDiskDefPtr def) } } } - volimg = uri->path + 1; /* skip the prefix slash */ - VIR_FREE(def->src); - def->src = strdup(volimg); - if (!def->src) - goto no_memory; + if (uri->path) { + volimg = uri->path + 1; /* skip the prefix slash */ + VIR_FREE(def->src); + def->src = strdup(volimg); + if (!def->src) + goto no_memory; + } else { + VIR_FREE(def->src); + def->src = NULL; + } def->nhosts = 1; ret = 0; @@ -2128,12 +2134,31 @@ error: } static int +qemuParseGlusterString(virDomainDiskDefPtr def) +{ + virURIPtr uri = NULL; + + if (!(uri = virURIParse(def->src))) + return -1; + + return qemuParseDriveURIString(def, uri, "gluster"); +} + +static int qemuParseNBDString(virDomainDiskDefPtr disk) { virDomainDiskHostDefPtr h = NULL; char *host, *port; char *src; + virURIPtr uri = NULL; + + if (strstr(disk->src, "://")) { + uri = virURIParse(disk->src); + if (uri) + return qemuParseDriveURIString(disk, uri, "nbd"); + } + if (VIR_ALLOC(h) < 0) goto no_memory; @@ -2190,7 +2215,8 @@ error: } static int -qemuBuildGlusterString(virDomainDiskDefPtr disk, virBufferPtr opt) +qemuBuildDriveURIString(virDomainDiskDefPtr disk, virBufferPtr opt, + const char *scheme) { int ret = -1; int port = 0; @@ -2204,18 +2230,18 @@ qemuBuildGlusterString(virDomainDiskDefPtr disk, virBufferPtr opt) }; if (disk->nhosts != 1) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("gluster accepts only one host")); + virReportError(VIR_ERR_INTERNAL_ERROR, + _("%s accepts only one host"), scheme); return -1; } virBufferAddLit(opt, "file="); transp = virDomainDiskProtocolTransportTypeToString(disk->hosts->transport); - if (virAsprintf(&tmpscheme, "gluster+%s", transp) < 0) + if (virAsprintf(&tmpscheme, "%s+%s", scheme, transp) < 0) goto no_memory; - if (virAsprintf(&volimg, "/%s", disk->src) < 0) + if (disk->src && virAsprintf(&volimg, "/%s", disk->src) < 0) goto no_memory; if (disk->hosts->port) { @@ -2251,16 +2277,29 @@ no_memory: } static int +qemuBuildGlusterString(virDomainDiskDefPtr disk, virBufferPtr opt) +{ + return qemuBuildDriveURIString(disk, opt, "gluster"); +} + +static int qemuBuildNBDString(virDomainDiskDefPtr disk, virBufferPtr opt) { const char *transp; if (disk->nhosts != 1) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("nbd accepts only one host")); + virReportError(VIR_ERR_INTERNAL_ERROR, + _("%s accepts only one host"), "nbd"); return -1; } + if ((disk->hosts->name && strchr(disk->hosts->name, ':')) + || (disk->hosts->transport == VIR_DOMAIN_DISK_PROTO_TRANS_TCP + && !disk->hosts->name) + || (disk->hosts->transport == VIR_DOMAIN_DISK_PROTO_TRANS_UNIX + && disk->hosts->socket && disk->hosts->socket[0] != '/')) + return qemuBuildDriveURIString(disk, opt, "nbd"); + virBufferAddLit(opt, "file=nbd:"); switch (disk->hosts->transport) { @@ -7435,7 +7474,8 @@ qemuParseCommandLineDisk(virCapsPtr qemuCaps, values[i] = NULL; if (STRPREFIX(def->src, "/dev/")) def->type = VIR_DOMAIN_DISK_TYPE_BLOCK; - else if (STRPREFIX(def->src, "nbd:")) { + else if (STRPREFIX(def->src, "nbd:") || + STRPREFIX(def->src, "nbd+")) { def->type = VIR_DOMAIN_DISK_TYPE_NETWORK; def->protocol = VIR_DOMAIN_DISK_PROTOCOL_NBD; @@ -7456,7 +7496,8 @@ qemuParseCommandLineDisk(virCapsPtr qemuCaps, goto cleanup; VIR_FREE(p); - } else if (STRPREFIX(def->src, "gluster")) { + } else if (STRPREFIX(def->src, "gluster:") || + STRPREFIX(def->src, "gluster+")) { def->type = VIR_DOMAIN_DISK_TYPE_NETWORK; def->protocol = VIR_DOMAIN_DISK_PROTOCOL_GLUSTER; diff --git a/tests/qemuargv2xmltest.c b/tests/qemuargv2xmltest.c index 12174b3..9fbba94 100644 --- a/tests/qemuargv2xmltest.c +++ b/tests/qemuargv2xmltest.c @@ -183,6 +183,8 @@ mymain(void) DO_TEST("disk-drive-cache-unsafe"); DO_TEST("disk-drive-network-nbd"); DO_TEST("disk-drive-network-nbd-export"); + DO_TEST("disk-drive-network-nbd-ipv6"); + DO_TEST("disk-drive-network-nbd-ipv6-export"); DO_TEST("disk-drive-network-nbd-unix"); DO_TEST("disk-drive-network-gluster"); DO_TEST("disk-drive-network-rbd"); diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-nbd-ipv6-export.args b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-nbd-ipv6-export.args new file mode 100644 index 0000000..e12d4d6 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-nbd-ipv6-export.args @@ -0,0 +1,5 @@ +LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test /usr/bin/qemu -S -M \ +pc -m 214 -smp 1 -nographic -monitor unix:/tmp/test-monitor,server,nowait \ +-no-acpi -boot c -usb -drive file=/dev/HostVG/QEMUGuest1,if=ide,bus=0,unit=0 -drive \ +'file=nbd+tcp://[::1]:6000/bar,if=virtio,format=raw' -net none -serial none \ +-parallel none diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-nbd-ipv6-export.xml b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-nbd-ipv6-export.xml new file mode 100644 index 0000000..595d7ea --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-nbd-ipv6-export.xml @@ -0,0 +1,33 @@ +<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</emulator> + <disk type='block' device='disk'> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='hda' bus='ide'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> + <disk type='network' device='disk'> + <driver name='qemu' type='raw'/> + <source protocol='nbd' name='bar'> + <host name='::1' port='6000'/> + </source> + <target dev='vda' bus='virtio'/> + </disk> + <controller type='usb' index='0'/> + <controller type='ide' index='0'/> + <memballoon model='virtio'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-nbd-ipv6.args b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-nbd-ipv6.args new file mode 100644 index 0000000..e814805 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-nbd-ipv6.args @@ -0,0 +1,5 @@ +LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test /usr/bin/qemu -S -M \ +pc -m 214 -smp 1 -nographic -monitor unix:/tmp/test-monitor,server,nowait \ +-no-acpi -boot c -usb -drive file=/dev/HostVG/QEMUGuest1,if=ide,bus=0,unit=0 -drive \ +'file=nbd+tcp://[::1]:6000,if=virtio,format=raw' -net none -serial none \ +-parallel none diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-nbd-ipv6.xml b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-nbd-ipv6.xml new file mode 100644 index 0000000..3c5c99d --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-nbd-ipv6.xml @@ -0,0 +1,33 @@ +<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</emulator> + <disk type='block' device='disk'> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='hda' bus='ide'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> + <disk type='network' device='disk'> + <driver name='qemu' type='raw'/> + <source protocol='nbd'> + <host name='::1' port='6000'/> + </source> + <target dev='vda' bus='virtio'/> + </disk> + <controller type='usb' index='0'/> + <controller type='ide' index='0'/> + <memballoon model='virtio'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index db09633..35cba01 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -487,6 +487,10 @@ mymain(void) QEMU_CAPS_DRIVE, QEMU_CAPS_DRIVE_FORMAT); DO_TEST("disk-drive-network-nbd-export", QEMU_CAPS_DRIVE, QEMU_CAPS_DRIVE_FORMAT); + DO_TEST("disk-drive-network-nbd-ipv6", + QEMU_CAPS_DRIVE, QEMU_CAPS_DRIVE_FORMAT); + DO_TEST("disk-drive-network-nbd-ipv6-export", + QEMU_CAPS_DRIVE, QEMU_CAPS_DRIVE_FORMAT); DO_TEST("disk-drive-network-nbd-unix", QEMU_CAPS_DRIVE, QEMU_CAPS_DRIVE_FORMAT); DO_TEST("disk-drive-network-gluster", diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index d698f59..7a01954 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -168,6 +168,8 @@ mymain(void) DO_TEST("disk-drive-cache-v1-none"); DO_TEST("disk-drive-network-nbd"); DO_TEST("disk-drive-network-nbd-export"); + DO_TEST("disk-drive-network-nbd-ipv6"); + DO_TEST("disk-drive-network-nbd-ipv6-export"); DO_TEST("disk-drive-network-nbd-unix"); DO_TEST("disk-scsi-device"); DO_TEST("disk-scsi-vscsi"); -- 1.8.1.2
On 2013年02月26日 01:44, Paolo Bonzini wrote:
QEMU 1.3 and newer support an alternative URI-based syntax to specify the location of an NBD server. Libvirt can keep on using the old syntax in general, but only the URI syntax supports IPv6 addresses.
The URI syntax also supports relative paths to Unix sockets. These should never be used but aren't explicitly blocked either by the parser, so support it just in case.
The URI syntax is intentionally compatible with Gluster's, and the code can be reused.
Signed-off-by: Paolo Bonzini<pbonzini@redhat.com> --- src/qemu/qemu_command.c | 97 +++++++++++++++------- tests/qemuargv2xmltest.c | 2 + ...ml2argv-disk-drive-network-nbd-ipv6-export.args | 5 ++ ...xml2argv-disk-drive-network-nbd-ipv6-export.xml | 33 ++++++++ .../qemuxml2argv-disk-drive-network-nbd-ipv6.args | 5 ++ .../qemuxml2argv-disk-drive-network-nbd-ipv6.xml | 33 ++++++++ tests/qemuxml2argvtest.c | 4 + tests/qemuxml2xmltest.c | 2 + 8 files changed, 153 insertions(+), 28 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-nbd-ipv6-export.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-nbd-ipv6-export.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-nbd-ipv6.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-nbd-ipv6.xml
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 89cd065..733d3bf 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -2050,35 +2050,36 @@ no_memory: }
static int -qemuParseGlusterString(virDomainDiskDefPtr def) +qemuParseDriveURIString(virDomainDiskDefPtr def, virURIPtr uri, + const char *scheme) { int ret = -1; char *transp = NULL; char *sock = NULL; char *volimg = NULL; - virURIPtr uri = NULL; - - if (!(uri = virURIParse(def->src))) { - return -1; - }
if (VIR_ALLOC(def->hosts)< 0) goto no_memory;
- if (STREQ(uri->scheme, "gluster")) { + transp = strchr(uri->scheme, '+'); + if (transp) + *transp++ = 0; + + if (!STREQ(uri->scheme, scheme)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Invalid transport/scheme '%s'"), uri->scheme); + goto error; + }
How about uri->scheme is "gluster", and scheme is "nbd"? The error doesn't reflect the truth then. Except this and the coding style, all looks good. Osier
Il 11/03/2013 11:09, Osier Yang ha scritto:
+ if (!STREQ(uri->scheme, scheme)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Invalid transport/scheme '%s'"), uri->scheme); + goto error; + }
How about uri->scheme is "gluster", and scheme is "nbd"? The error doesn't reflect the truth then.
Except this and the coding style, all looks good.
The caller should guarantee that this does not happen. Paolo
On Mon, Feb 25, 2013 at 06:44:25PM +0100, Paolo Bonzini wrote:
QEMU 1.3 and newer support an alternative URI-based syntax to specify the location of an NBD server. Libvirt can keep on using the old syntax in general, but only the URI syntax supports IPv6 addresses.
The URI syntax also supports relative paths to Unix sockets. These should never be used but aren't explicitly blocked either by the parser, so support it just in case.
The URI syntax is intentionally compatible with Gluster's, and the code can be reused.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- src/qemu/qemu_command.c | 97 +++++++++++++++------- tests/qemuargv2xmltest.c | 2 + ...ml2argv-disk-drive-network-nbd-ipv6-export.args | 5 ++ ...xml2argv-disk-drive-network-nbd-ipv6-export.xml | 33 ++++++++ .../qemuxml2argv-disk-drive-network-nbd-ipv6.args | 5 ++ .../qemuxml2argv-disk-drive-network-nbd-ipv6.xml | 33 ++++++++ tests/qemuxml2argvtest.c | 4 + tests/qemuxml2xmltest.c | 2 + 8 files changed, 153 insertions(+), 28 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-nbd-ipv6-export.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-nbd-ipv6-export.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-nbd-ipv6.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-nbd-ipv6.xml
ACK Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 03/15/2013 08:36 AM, Daniel P. Berrange wrote:
On Mon, Feb 25, 2013 at 06:44:25PM +0100, Paolo Bonzini wrote:
QEMU 1.3 and newer support an alternative URI-based syntax to specify the location of an NBD server. Libvirt can keep on using the old syntax in general, but only the URI syntax supports IPv6 addresses.
The URI syntax also supports relative paths to Unix sockets. These should never be used but aren't explicitly blocked either by the parser, so support it just in case.
create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-nbd-ipv6-export.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-nbd-ipv6-export.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-nbd-ipv6.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-nbd-ipv6.xml
Again, I rewrapped test files...
ACK
and pushed. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
This plumbs in the XML description of iSCSI shares. The next patches will add support for the libiscsi userspace initiator. Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- docs/formatdomain.html.in | 7 +++++- docs/schemas/domaincommon.rng | 1 + src/conf/domain_conf.c | 3 ++- src/conf/domain_conf.h | 1 + .../qemuxml2argv-disk-drive-network-iscsi.xml | 27 ++++++++++++++++++++++ tests/qemuxml2xmltest.c | 1 + 6 files changed, 38 insertions(+), 2 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi.xml diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 094b509..fd6d5ae 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -1438,7 +1438,7 @@ to the directory to use as the disk. If the disk <code>type</code> is "network", then the <code>protocol</code> attribute specifies the protocol to access to the requested image; possible values - are "nbd", "rbd", "sheepdog" or "gluster". If the + 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 mandatory to specify which volume/image will be used; for "nbd" it is optional. When the disk @@ -1698,6 +1698,11 @@ <td> only one </td> </tr> <tr> + <td> iscsi </td> + <td> an iSCSI server </td> + <td> only one </td> + </tr> + <tr> <td> rbd </td> <td> monitor servers of RBD </td> <td> one or more </td> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 63be4aa..980410f 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -1084,6 +1084,7 @@ <value>rbd</value> <value>sheepdog</value> <value>gluster</value> + <value>iscsi</value> </choice> </attribute> <optional> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 0c75838..b42c79c 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -235,7 +235,8 @@ VIR_ENUM_IMPL(virDomainDiskProtocol, VIR_DOMAIN_DISK_PROTOCOL_LAST, "nbd", "rbd", "sheepdog", - "gluster") + "gluster", + "iscsi") VIR_ENUM_IMPL(virDomainDiskProtocolTransport, VIR_DOMAIN_DISK_PROTO_TRANS_LAST, "tcp", diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 4ffa4aa..7cd0264 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -494,6 +494,7 @@ enum virDomainDiskProtocol { VIR_DOMAIN_DISK_PROTOCOL_RBD, VIR_DOMAIN_DISK_PROTOCOL_SHEEPDOG, VIR_DOMAIN_DISK_PROTOCOL_GLUSTER, + VIR_DOMAIN_DISK_PROTOCOL_ISCSI, VIR_DOMAIN_DISK_PROTOCOL_LAST }; diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi.xml b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi.xml new file mode 100644 index 0000000..dd85032 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi.xml @@ -0,0 +1,27 @@ +<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</emulator> + <disk type='network' device='disk'> + <driver name='qemu' type='raw'/> + <source protocol='iscsi' name='iqn.1992-01.com.example'> + <host name='example.org' port='6000'/> + </source> + <target dev='vda' bus='virtio'/> + </disk> + <controller type='usb' index='0'/> + <memballoon model='virtio'/> + </devices> +</domain> diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index 7a01954..e0d3b20 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -171,6 +171,7 @@ mymain(void) DO_TEST("disk-drive-network-nbd-ipv6"); DO_TEST("disk-drive-network-nbd-ipv6-export"); DO_TEST("disk-drive-network-nbd-unix"); + DO_TEST("disk-drive-network-iscsi"); DO_TEST("disk-scsi-device"); DO_TEST("disk-scsi-vscsi"); DO_TEST("disk-scsi-virtio-scsi"); -- 1.8.1.2
On 2013年02月26日 01:44, Paolo Bonzini wrote:
This plumbs in the XML description of iSCSI shares. The next patches will add support for the libiscsi userspace initiator.
Signed-off-by: Paolo Bonzini<pbonzini@redhat.com> --- docs/formatdomain.html.in | 7 +++++- docs/schemas/domaincommon.rng | 1 + src/conf/domain_conf.c | 3 ++- src/conf/domain_conf.h | 1 + .../qemuxml2argv-disk-drive-network-iscsi.xml | 27 ++++++++++++++++++++++ tests/qemuxml2xmltest.c | 1 + 6 files changed, 38 insertions(+), 2 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi.xml
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 094b509..fd6d5ae 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -1438,7 +1438,7 @@ to the directory to use as the disk. If the disk<code>type</code> is "network", then the<code>protocol</code> attribute specifies the protocol to access to the requested image; possible values - are "nbd", "rbd", "sheepdog" or "gluster". If the + are "nbd", "iscsi", "rbd", "sheepdog" or "gluster". If the
Worth to add "<span class="since">since 1.0.4</span>"? ACK with the question kept.
On 03/11/2013 04:19 AM, Osier Yang wrote:
On 2013年02月26日 01:44, Paolo Bonzini wrote:
This plumbs in the XML description of iSCSI shares. The next patches will add support for the libiscsi userspace initiator.
Signed-off-by: Paolo Bonzini<pbonzini@redhat.com> ---
the protocol to access to the requested image; possible values - are "nbd", "rbd", "sheepdog" or "gluster". If the + are "nbd", "iscsi", "rbd", "sheepdog" or "gluster". If the
Worth to add "<span class="since">since 1.0.4</span>"?
Yes, as follows: diff --git i/docs/formatdomain.html.in w/docs/formatdomain.html.in index 397836e..e4ed3f7 100644 --- i/docs/formatdomain.html.in +++ w/docs/formatdomain.html.in @@ -1451,7 +1451,8 @@ or more <code>host</code> sub-elements used to specify the hosts to connect. <span class="since">Since 0.0.3; <code>type='dir'</code> since - 0.7.5; <code>type='network'</code> since 0.8.7</span><br/> + 0.7.5; <code>type='network'</code> since + 0.8.7; <code>protocol='iscsi'</code> since 1.0.4</span><br/> For a "file" disk type which represents a cdrom or floppy (the <code>device</code> attribute), it is possible to define policy what to do with the disk if the source file is not accessible.
ACK with the question kept.
Pushed, and sorry for the delay (I had started pushing the acked portion of the series last week, then got interrupted halfway through). -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Mon, Feb 25, 2013 at 06:44:26PM +0100, Paolo Bonzini wrote:
This plumbs in the XML description of iSCSI shares. The next patches will add support for the libiscsi userspace initiator.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- docs/formatdomain.html.in | 7 +++++- docs/schemas/domaincommon.rng | 1 + src/conf/domain_conf.c | 3 ++- src/conf/domain_conf.h | 1 + .../qemuxml2argv-disk-drive-network-iscsi.xml | 27 ++++++++++++++++++++++ tests/qemuxml2xmltest.c | 1 + 6 files changed, 38 insertions(+), 2 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi.xml
ACK Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
libiscsi provides a userspace iSCSI initiator. The main advantage over the kernel initiator is that it is very easy to provide different initiator names for VMs on the same host. Thus libiscsi supports usage of persistent reservations in the VM, which otherwise would only be possible with NPIV. libiscsi uses "iscsi" as the scheme, not "iscsi+tcp". We can change this in the tests (while remaining backwards-compatible manner, because QEMU uses TCP as the default transport for both Gluster and NBD). Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- src/qemu/qemu_command.c | 49 +++++++++++++++++++++- tests/qemuargv2xmltest.c | 1 + .../qemuxml2argv-disk-drive-network-gluster.args | 2 +- .../qemuxml2argv-disk-drive-network-iscsi.args | 1 + ...ml2argv-disk-drive-network-nbd-ipv6-export.args | 2 +- .../qemuxml2argv-disk-drive-network-nbd-ipv6.args | 2 +- tests/qemuxml2argvtest.c | 2 + 7 files changed, 54 insertions(+), 5 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi.args diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 733d3bf..07700cc 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -2145,6 +2145,23 @@ qemuParseGlusterString(virDomainDiskDefPtr def) } static int +qemuParseISCSIString(virDomainDiskDefPtr def) +{ + virURIPtr uri = NULL; + + if (!(uri = virURIParse(def->src))) + return -1; + + if (uri->path && strchr(uri->path + 1, '/')) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("invalid address for iSCSI target"), disk->src); + return -1; + } + + return qemuParseDriveURIString(def, uri, "iscsi"); +} + +static int qemuParseNBDString(virDomainDiskDefPtr disk) { virDomainDiskHostDefPtr h = NULL; @@ -2238,8 +2255,14 @@ qemuBuildDriveURIString(virDomainDiskDefPtr disk, virBufferPtr opt, virBufferAddLit(opt, "file="); transp = virDomainDiskProtocolTransportTypeToString(disk->hosts->transport); - if (virAsprintf(&tmpscheme, "%s+%s", scheme, transp) < 0) - goto no_memory; + if (disk->hosts->transport == VIR_DOMAIN_DISK_PROTO_TRANS_TCP) { + tmpscheme = strdup(scheme); + if (tmpscheme == NULL) + goto no_memory; + } else { + if (virAsprintf(&tmpscheme, "%s+%s", scheme, transp) < 0) + goto no_memory; + } if (disk->src && virAsprintf(&volimg, "/%s", disk->src) < 0) goto no_memory; @@ -2283,6 +2306,12 @@ qemuBuildGlusterString(virDomainDiskDefPtr disk, virBufferPtr opt) } static int +qemuBuildISCSIString(virDomainDiskDefPtr disk, virBufferPtr opt) +{ + return qemuBuildDriveURIString(disk, opt, "iscsi"); +} + +static int qemuBuildNBDString(virDomainDiskDefPtr disk, virBufferPtr opt) { const char *transp; @@ -2471,6 +2500,11 @@ qemuBuildDriveStr(virConnectPtr conn ATTRIBUTE_UNUSED, goto error; virBufferAddChar(&opt, ','); break; + case VIR_DOMAIN_DISK_PROTOCOL_ISCSI: + if (qemuBuildISCSIString(disk, &opt) < 0) + goto error; + virBufferAddChar(&opt, ','); + break; case VIR_DOMAIN_DISK_PROTOCOL_SHEEPDOG: if (disk->nhosts == 0) { @@ -7503,6 +7537,12 @@ qemuParseCommandLineDisk(virCapsPtr qemuCaps, if (qemuParseGlusterString(def) < 0) goto error; + } else if (STRPREFIX(def->src, "iscsi:")) { + def->type = VIR_DOMAIN_DISK_TYPE_NETWORK; + def->protocol = VIR_DOMAIN_DISK_PROTOCOL_ISCSI; + + if (qemuParseISCSIString(def) < 0) + goto error; } else if (STRPREFIX(def->src, "sheepdog:")) { char *p = def->src; char *port, *vdi; @@ -8793,6 +8833,11 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr qemuCaps, goto error; break; + case VIR_DOMAIN_DISK_PROTOCOL_ISCSI: + if (qemuParseISCSIString(disk) < 0) + goto error; + + break; } } diff --git a/tests/qemuargv2xmltest.c b/tests/qemuargv2xmltest.c index 9fbba94..314c7b3 100644 --- a/tests/qemuargv2xmltest.c +++ b/tests/qemuargv2xmltest.c @@ -186,6 +186,7 @@ mymain(void) DO_TEST("disk-drive-network-nbd-ipv6"); DO_TEST("disk-drive-network-nbd-ipv6-export"); DO_TEST("disk-drive-network-nbd-unix"); + DO_TEST("disk-drive-network-iscsi"); DO_TEST("disk-drive-network-gluster"); DO_TEST("disk-drive-network-rbd"); DO_TEST("disk-drive-network-rbd-ipv6"); diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-gluster.args b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-gluster.args index 6dbb009..9d70586 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-gluster.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-gluster.args @@ -1 +1 @@ -LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test /usr/bin/qemu -S -M pc -m 214 -smp 1 -nographic -monitor unix:/tmp/test-monitor,server,nowait -no-acpi -boot c -usb -drive file=gluster+tcp://example.org:6000/Volume1/Image,if=virtio,format=raw -drive 'file=gluster+unix:///Volume2/Image?socket=/path/to/sock,if=virtio,format=raw' -net none -serial none -parallel none +LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test /usr/bin/qemu -S -M pc -m 214 -smp 1 -nographic -monitor unix:/tmp/test-monitor,server,nowait -no-acpi -boot c -usb -drive file=gluster://example.org:6000/Volume1/Image,if=virtio,format=raw -drive 'file=gluster+unix:///Volume2/Image?socket=/path/to/sock,if=virtio,format=raw' -net none -serial none -parallel none diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi.args b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi.args new file mode 100644 index 0000000..ed4f337 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi.args @@ -0,0 +1 @@ +LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test /usr/bin/qemu -S -M pc -m 214 -smp 1 -nographic -monitor unix:/tmp/test-monitor,server,nowait -no-acpi -boot c -usb -drive file=iscsi://example.org:6000/iqn.1992-01.com.example,if=virtio,format=raw -net none -serial none -parallel none diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-nbd-ipv6-export.args b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-nbd-ipv6-export.args index e12d4d6..183119b 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-nbd-ipv6-export.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-nbd-ipv6-export.args @@ -1,5 +1,5 @@ LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test /usr/bin/qemu -S -M \ pc -m 214 -smp 1 -nographic -monitor unix:/tmp/test-monitor,server,nowait \ -no-acpi -boot c -usb -drive file=/dev/HostVG/QEMUGuest1,if=ide,bus=0,unit=0 -drive \ -'file=nbd+tcp://[::1]:6000/bar,if=virtio,format=raw' -net none -serial none \ +'file=nbd://[::1]:6000/bar,if=virtio,format=raw' -net none -serial none \ -parallel none diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-nbd-ipv6.args b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-nbd-ipv6.args index e814805..a0cb07b 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-nbd-ipv6.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-nbd-ipv6.args @@ -1,5 +1,5 @@ LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test /usr/bin/qemu -S -M \ pc -m 214 -smp 1 -nographic -monitor unix:/tmp/test-monitor,server,nowait \ -no-acpi -boot c -usb -drive file=/dev/HostVG/QEMUGuest1,if=ide,bus=0,unit=0 -drive \ -'file=nbd+tcp://[::1]:6000,if=virtio,format=raw' -net none -serial none \ +'file=nbd://[::1]:6000,if=virtio,format=raw' -net none -serial none \ -parallel none diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 35cba01..babdd8c 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -493,6 +493,8 @@ mymain(void) QEMU_CAPS_DRIVE, QEMU_CAPS_DRIVE_FORMAT); DO_TEST("disk-drive-network-nbd-unix", QEMU_CAPS_DRIVE, QEMU_CAPS_DRIVE_FORMAT); + DO_TEST("disk-drive-network-iscsi", + QEMU_CAPS_DRIVE, QEMU_CAPS_DRIVE_FORMAT); DO_TEST("disk-drive-network-gluster", QEMU_CAPS_DRIVE, QEMU_CAPS_DRIVE_FORMAT); DO_TEST("disk-drive-network-rbd", -- 1.8.1.2
On 2013年02月26日 01:44, Paolo Bonzini wrote:
libiscsi provides a userspace iSCSI initiator.
The main advantage over the kernel initiator is that it is very easy to provide different initiator names for VMs on the same host. Thus libiscsi supports usage of persistent reservations in the VM, which otherwise would only be possible with NPIV.
libiscsi uses "iscsi" as the scheme, not "iscsi+tcp". We can change this in the tests (while remaining backwards-compatible manner, because QEMU uses TCP as the default transport for both Gluster and NBD).
Signed-off-by: Paolo Bonzini<pbonzini@redhat.com> --- src/qemu/qemu_command.c | 49 +++++++++++++++++++++- tests/qemuargv2xmltest.c | 1 + .../qemuxml2argv-disk-drive-network-gluster.args | 2 +- .../qemuxml2argv-disk-drive-network-iscsi.args | 1 + ...ml2argv-disk-drive-network-nbd-ipv6-export.args | 2 +- .../qemuxml2argv-disk-drive-network-nbd-ipv6.args | 2 +- tests/qemuxml2argvtest.c | 2 + 7 files changed, 54 insertions(+), 5 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi.args
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 733d3bf..07700cc 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -2145,6 +2145,23 @@ qemuParseGlusterString(virDomainDiskDefPtr def) }
static int +qemuParseISCSIString(virDomainDiskDefPtr def) +{ + virURIPtr uri = NULL; + + if (!(uri = virURIParse(def->src))) + return -1; + + if (uri->path&& strchr(uri->path + 1, '/')) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("invalid address for iSCSI target"), disk->src); + return -1; + } + + return qemuParseDriveURIString(def, uri, "iscsi"); +} + +static int qemuParseNBDString(virDomainDiskDefPtr disk) { virDomainDiskHostDefPtr h = NULL; @@ -2238,8 +2255,14 @@ qemuBuildDriveURIString(virDomainDiskDefPtr disk, virBufferPtr opt, virBufferAddLit(opt, "file="); transp = virDomainDiskProtocolTransportTypeToString(disk->hosts->transport);
- if (virAsprintf(&tmpscheme, "%s+%s", scheme, transp)< 0) - goto no_memory; + if (disk->hosts->transport == VIR_DOMAIN_DISK_PROTO_TRANS_TCP) { + tmpscheme = strdup(scheme); + if (tmpscheme == NULL) + goto no_memory;
Why not keeping the "+tcp" for Gluster and NBD instead? I'm afraid of of relying on qemu's default. Can it be changed? Osier
Il 11/03/2013 11:44, Osier Yang ha scritto:
On 2013年02月26日 01:44, Paolo Bonzini wrote:
libiscsi provides a userspace iSCSI initiator.
The main advantage over the kernel initiator is that it is very easy to provide different initiator names for VMs on the same host. Thus libiscsi supports usage of persistent reservations in the VM, which otherwise would only be possible with NPIV.
libiscsi uses "iscsi" as the scheme, not "iscsi+tcp". We can change this in the tests (while remaining backwards-compatible manner, because QEMU uses TCP as the default transport for both Gluster and NBD).
Signed-off-by: Paolo Bonzini<pbonzini@redhat.com> --- src/qemu/qemu_command.c | 49 +++++++++++++++++++++- tests/qemuargv2xmltest.c | 1 + .../qemuxml2argv-disk-drive-network-gluster.args | 2 +- .../qemuxml2argv-disk-drive-network-iscsi.args | 1 + ...ml2argv-disk-drive-network-nbd-ipv6-export.args | 2 +- .../qemuxml2argv-disk-drive-network-nbd-ipv6.args | 2 +- tests/qemuxml2argvtest.c | 2 + 7 files changed, 54 insertions(+), 5 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi.args
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 733d3bf..07700cc 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -2145,6 +2145,23 @@ qemuParseGlusterString(virDomainDiskDefPtr def) }
static int +qemuParseISCSIString(virDomainDiskDefPtr def) +{ + virURIPtr uri = NULL; + + if (!(uri = virURIParse(def->src))) + return -1; + + if (uri->path&& strchr(uri->path + 1, '/')) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("invalid address for iSCSI target"), disk->src); + return -1; + } + + return qemuParseDriveURIString(def, uri, "iscsi"); +} + +static int qemuParseNBDString(virDomainDiskDefPtr disk) { virDomainDiskHostDefPtr h = NULL; @@ -2238,8 +2255,14 @@ qemuBuildDriveURIString(virDomainDiskDefPtr disk, virBufferPtr opt, virBufferAddLit(opt, "file="); transp = virDomainDiskProtocolTransportTypeToString(disk->hosts->transport);
- if (virAsprintf(&tmpscheme, "%s+%s", scheme, transp)< 0) - goto no_memory; + if (disk->hosts->transport == VIR_DOMAIN_DISK_PROTO_TRANS_TCP) { + tmpscheme = strdup(scheme); + if (tmpscheme == NULL) + goto no_memory;
Why not keeping the "+tcp" for Gluster and NBD instead?
Because it simplifies the code.
I'm afraid of of relying on qemu's default. Can it be changed?
No, it's API. Paolo
On 2013年03月11日 21:22, Paolo Bonzini wrote:
Il 11/03/2013 11:44, Osier Yang ha scritto:
On 2013年02月26日 01:44, Paolo Bonzini wrote:
libiscsi provides a userspace iSCSI initiator.
The main advantage over the kernel initiator is that it is very easy to provide different initiator names for VMs on the same host. Thus libiscsi supports usage of persistent reservations in the VM, which otherwise would only be possible with NPIV.
libiscsi uses "iscsi" as the scheme, not "iscsi+tcp". We can change this in the tests (while remaining backwards-compatible manner, because QEMU uses TCP as the default transport for both Gluster and NBD).
Signed-off-by: Paolo Bonzini<pbonzini@redhat.com> --- src/qemu/qemu_command.c | 49 +++++++++++++++++++++- tests/qemuargv2xmltest.c | 1 + .../qemuxml2argv-disk-drive-network-gluster.args | 2 +- .../qemuxml2argv-disk-drive-network-iscsi.args | 1 + ...ml2argv-disk-drive-network-nbd-ipv6-export.args | 2 +- .../qemuxml2argv-disk-drive-network-nbd-ipv6.args | 2 +- tests/qemuxml2argvtest.c | 2 + 7 files changed, 54 insertions(+), 5 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi.args
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 733d3bf..07700cc 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -2145,6 +2145,23 @@ qemuParseGlusterString(virDomainDiskDefPtr def) }
static int +qemuParseISCSIString(virDomainDiskDefPtr def) +{ + virURIPtr uri = NULL; + + if (!(uri = virURIParse(def->src))) + return -1; + + if (uri->path&& strchr(uri->path + 1, '/')) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("invalid address for iSCSI target"), disk->src); + return -1; + } + + return qemuParseDriveURIString(def, uri, "iscsi"); +} + +static int qemuParseNBDString(virDomainDiskDefPtr disk) { virDomainDiskHostDefPtr h = NULL; @@ -2238,8 +2255,14 @@ qemuBuildDriveURIString(virDomainDiskDefPtr disk, virBufferPtr opt, virBufferAddLit(opt, "file="); transp = virDomainDiskProtocolTransportTypeToString(disk->hosts->transport);
- if (virAsprintf(&tmpscheme, "%s+%s", scheme, transp)< 0) - goto no_memory; + if (disk->hosts->transport == VIR_DOMAIN_DISK_PROTO_TRANS_TCP) { + tmpscheme = strdup(scheme); + if (tmpscheme == NULL) + goto no_memory;
Why not keeping the "+tcp" for Gluster and NBD instead?
Because it simplifies the code.
I'm afraid of of relying on qemu's default. Can it be changed?
No, it's API.
Okay, I believe you, ACK.
On Mon, Feb 25, 2013 at 06:44:27PM +0100, Paolo Bonzini wrote:
libiscsi provides a userspace iSCSI initiator.
The main advantage over the kernel initiator is that it is very easy to provide different initiator names for VMs on the same host. Thus libiscsi supports usage of persistent reservations in the VM, which otherwise would only be possible with NPIV.
libiscsi uses "iscsi" as the scheme, not "iscsi+tcp". We can change this in the tests (while remaining backwards-compatible manner, because QEMU uses TCP as the default transport for both Gluster and NBD).
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- src/qemu/qemu_command.c | 49 +++++++++++++++++++++- tests/qemuargv2xmltest.c | 1 + .../qemuxml2argv-disk-drive-network-gluster.args | 2 +- .../qemuxml2argv-disk-drive-network-iscsi.args | 1 + ...ml2argv-disk-drive-network-nbd-ipv6-export.args | 2 +- .../qemuxml2argv-disk-drive-network-nbd-ipv6.args | 2 +- tests/qemuxml2argvtest.c | 2 + 7 files changed, 54 insertions(+), 5 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi.args
ACK Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 03/15/2013 08:39 AM, Daniel P. Berrange wrote:
On Mon, Feb 25, 2013 at 06:44:27PM +0100, Paolo Bonzini wrote:
libiscsi provides a userspace iSCSI initiator.
The main advantage over the kernel initiator is that it is very easy to provide different initiator names for VMs on the same host. Thus libiscsi supports usage of persistent reservations in the VM, which otherwise would only be possible with NPIV.
libiscsi uses "iscsi" as the scheme, not "iscsi+tcp". We can change this in the tests (while remaining backwards-compatible manner, because QEMU uses TCP as the default transport for both Gluster and NBD).
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- src/qemu/qemu_command.c | 49 +++++++++++++++++++++- tests/qemuargv2xmltest.c | 1 + .../qemuxml2argv-disk-drive-network-gluster.args | 2 +- .../qemuxml2argv-disk-drive-network-iscsi.args | 1 + ...ml2argv-disk-drive-network-nbd-ipv6-export.args | 2 +- .../qemuxml2argv-disk-drive-network-nbd-ipv6.args | 2 +- tests/qemuxml2argvtest.c | 2 + 7 files changed, 54 insertions(+), 5 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi.args
ACK
I'm starting to get merge conflicts while trying to apply this, after the tweaks that have been made to earlier patches in the series. Would you mind rebasing and reposting the remainder of this series? -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Each iSCSI target can provide multiple logical units. Support this with an additional attribute in the <source> element. libiscsi places the LUN in the path component of the URI. iSCSI names cannot contain slashes, so this is unambiguous Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- docs/formatdomain.html.in | 9 ++++--- docs/schemas/domaincommon.rng | 29 +++++++++++++++------- src/conf/domain_conf.c | 12 +++++++++ src/conf/domain_conf.h | 2 ++ src/qemu/qemu_command.c | 23 +++++++++++++---- .../qemuxml2argv-disk-drive-network-iscsi.args | 2 +- .../qemuxml2argv-disk-drive-network-iscsi.xml | 7 ++++++ 7 files changed, 65 insertions(+), 19 deletions(-) diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index fd6d5ae..5b4eabe 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -1441,10 +1441,11 @@ 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 mandatory to specify which - volume/image will be used; for "nbd" it is optional. When the disk - <code>type</code> is "network", the <code>source</code> may have zero - or more <code>host</code> sub-elements used to specify the hosts - to connect. + volume/image will be used; for "nbd" it is optional. "iscsi" supports + an additional numeric attribute <code>unit</code> for the logical unit + number (default 0). When the disk <code>type</code> is "network", + the <code>source</code> may have zero or more <code>host</code> + sub-elements used to specify the hosts to connect. <span class="since">Since 0.0.3; <code>type='dir'</code> since 0.7.5; <code>type='network'</code> since 0.8.7</span><br/> For a "file" disk type which represents a cdrom or floppy diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 980410f..82062a3 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -1078,15 +1078,26 @@ <interleave> <optional> <element name="source"> - <attribute name="protocol"> - <choice> - <value>nbd</value> - <value>rbd</value> - <value>sheepdog</value> - <value>gluster</value> - <value>iscsi</value> - </choice> - </attribute> + <choice> + <attribute name="protocol"> + <choice> + <value>nbd</value> + <value>rbd</value> + <value>sheepdog</value> + <value>gluster</value> + </choice> + </attribute> + <group> + <attribute name="protocol"> + <value>iscsi</value> + </attribute> + <optional> + <attribute name="unit"> + <data type="integer"/> + </attribute> + </optional> + </group> + </choice> <optional> <attribute name="name"/> </optional> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index b42c79c..b801239 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -3950,6 +3950,15 @@ virDomainDiskDefParseXML(virCapsPtr caps, protocol); goto error; } + if (def->protocol == VIR_DOMAIN_DISK_PROTOCOL_ISCSI) { + if (virXMLPropString(cur, "unit") && + virXPathUInt("string(./source/@unit)", + ctxt, &def->unit) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("invalid logical unit number")); + goto error; + } + } if (!(source = virXMLPropString(cur, "name")) && def->protocol != VIR_DOMAIN_DISK_PROTOCOL_NBD) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", @@ -12555,6 +12564,9 @@ virDomainDiskDefFormat(virBufferPtr buf, if (def->src) { virBufferEscapeString(buf, " name='%s'", def->src); } + if (def->protocol == VIR_DOMAIN_DISK_PROTOCOL_ISCSI && def->unit) { + virBufferAsprintf(buf, " unit='%u'", def->unit); + } if (def->nhosts == 0) { virBufferAddLit(buf, "/>\n"); } else { diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 7cd0264..ac8a446 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -606,6 +606,8 @@ struct _virDomainDiskDef { int device; int bus; char *src; + unsigned unit; + char *dst; int tray_status; int protocol; diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 07700cc..59773ec 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -2148,14 +2148,19 @@ static int qemuParseISCSIString(virDomainDiskDefPtr def) { virURIPtr uri = NULL; + char *p; if (!(uri = virURIParse(def->src))) return -1; - if (uri->path && strchr(uri->path + 1, '/')) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("invalid address for iSCSI target"), disk->src); - return -1; + p = uri->path ? strchr(uri->path + 1, '/') : NULL; + if (p) { + *p++ = '\0'; + if (virStrToLong_ui(p, NULL, 10, &def->unit) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("cannot parse iSCSI LUN '%s'"), p); + return -1; + } } return qemuParseDriveURIString(def, uri, "iscsi"); @@ -2308,7 +2313,15 @@ qemuBuildGlusterString(virDomainDiskDefPtr disk, virBufferPtr opt) static int qemuBuildISCSIString(virDomainDiskDefPtr disk, virBufferPtr opt) { - return qemuBuildDriveURIString(disk, opt, "iscsi"); + int ret; + ret = qemuBuildDriveURIString(disk, opt, "iscsi"); + if (ret < 0) + return ret; + + if (disk->unit) { + virBufferAsprintf(opt, "/%u", disk->unit); + } + return 0; } static int diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi.args b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi.args index ed4f337..b8bc9c6 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi.args @@ -1 +1 @@ -LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test /usr/bin/qemu -S -M pc -m 214 -smp 1 -nographic -monitor unix:/tmp/test-monitor,server,nowait -no-acpi -boot c -usb -drive file=iscsi://example.org:6000/iqn.1992-01.com.example,if=virtio,format=raw -net none -serial none -parallel none +LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test /usr/bin/qemu -S -M pc -m 214 -smp 1 -nographic -monitor unix:/tmp/test-monitor,server,nowait -no-acpi -boot c -usb -drive file=iscsi://example.org:6000/iqn.1992-01.com.example,if=virtio,format=raw -drive file=iscsi://example.org:6000/iqn.1992-01.com.example/1,if=virtio,format=raw -net none -serial none -parallel none diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi.xml b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi.xml index dd85032..dfa60f3 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi.xml +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi.xml @@ -21,6 +21,13 @@ </source> <target dev='vda' bus='virtio'/> </disk> + <disk type='network' device='disk'> + <driver name='qemu' type='raw'/> + <source protocol='iscsi' name='iqn.1992-01.com.example' unit='1'> + <host name='example.org' port='6000'/> + </source> + <target dev='vdb' bus='virtio'/> + </disk> <controller type='usb' index='0'/> <memballoon model='virtio'/> </devices> -- 1.8.1.2
On Mon, Feb 25, 2013 at 06:44:28PM +0100, Paolo Bonzini wrote:
Each iSCSI target can provide multiple logical units. Support this with an additional attribute in the <source> element.
Hmm, this is kind of what the 'name' attribute is used for with RBD / Gluster. I tend to feel we should just use that rather than adding a new attribute. eg <source name="<IQN>/<LUN>"> Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
Il 15/03/2013 15:40, Daniel P. Berrange ha scritto:
Each iSCSI target can provide multiple logical units. Support this with an additional attribute in the <source> element. Hmm, this is kind of what the 'name' attribute is used for with RBD / Gluster. I tend to feel we should just use that rather than adding a new attribute. eg
<source name="<IQN>/<LUN>">
I think IQN and LUN are separate things, and best kept separate. For example, for each IQN you can only have (if you use -readconfig, which this series doesn't do) a single username/password/initiator-name triple. Note that the 'name' attribute is already used for the IQN. Paolo
On Fri, Mar 15, 2013 at 04:53:19PM +0100, Paolo Bonzini wrote:
Il 15/03/2013 15:40, Daniel P. Berrange ha scritto:
Each iSCSI target can provide multiple logical units. Support this with an additional attribute in the <source> element. Hmm, this is kind of what the 'name' attribute is used for with RBD / Gluster. I tend to feel we should just use that rather than adding a new attribute. eg
<source name="<IQN>/<LUN>">
I think IQN and LUN are separate things, and best kept separate. For example, for each IQN you can only have (if you use -readconfig, which this series doesn't do) a single username/password/initiator-name triple.
Note that the 'name' attribute is already used for the IQN.
Yes, but that doesn't make it right. The 'name' attribute is intended to uniquely identify the exported volume on the server. Neither IQN or LUN alone can uniquely identify the volume, so the "name" attribute should use a combination of the two. This is the same scenario you have with RBD, where you have a 'pool' and 'volume', so the "name" attribute uses the "<pool>/<volume>" syntax for identifying the export. In retrospect this combined syntax may have been better split up into separate attributes, but that's what we have, so we should follow this existing practice for iSCSI too. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
Il 15/03/2013 16:59, Daniel P. Berrange ha scritto:
I think IQN and LUN are separate things, and best kept separate. For example, for each IQN you can only have (if you use -readconfig, which this series doesn't do) a single username/password/initiator-name triple.
Note that the 'name' attribute is already used for the IQN. Yes, but that doesn't make it right. The 'name' attribute is intended to uniquely identify the exported volume on the server. Neither IQN or LUN alone can uniquely identify the volume, so the "name" attribute should use a combination of the two. This is the same scenario you have with RBD, where you have a 'pool' and 'volume', so the "name" attribute uses the "<pool>/<volume>" syntax for identifying the export. In retrospect this combined syntax may have been better split up into separate attributes, but that's what we have, so we should follow this existing practice for iSCSI too.
Ok, that's what I missed. It should just work in fact, and I'm explicitly forbidding slashes in patch 8/13; I just need to drop that test. Paolo
Only sheepdog actually required it in the code, and we can use 7000 as the default---the same value that QEMU uses for the simple "sheepdog:VOLUME" syntax. With this change, the schema can be fixed to allow no port. Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- docs/formatdomain.html.in | 6 ++++++ docs/schemas/domaincommon.rng | 8 +++++--- src/conf/domain_conf.c | 5 ----- src/qemu/qemu_command.c | 3 ++- 4 files changed, 13 insertions(+), 9 deletions(-) diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 5b4eabe..c590427 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -1692,31 +1692,37 @@ <th> Protocol </th> <th> Meaning </th> <th> Number of hosts </th> + <th> Default port </th> </tr> <tr> <td> nbd </td> <td> a server running nbd-server </td> <td> only one </td> + <td> 10809 </td> </tr> <tr> <td> iscsi </td> <td> an iSCSI server </td> <td> only one </td> + <td> 3260 </td> </tr> <tr> <td> rbd </td> <td> monitor servers of RBD </td> <td> one or more </td> + <td> 6789 </td> </tr> <tr> <td> sheepdog </td> <td> one of the sheepdog servers (default is localhost:7000) </td> <td> zero or one </td> + <td> 7000 </td> </tr> <tr> <td> gluster </td> <td> a server running glusterd daemon </td> <td> only one </td> + <td> 24007 </td> </tr> </table> gluster supports "tcp", "rdma", "unix" as valid values for the diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 82062a3..b8c4503 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -1119,9 +1119,11 @@ <ref name="ipAddr"/> </choice> </attribute> - <attribute name="port"> - <ref name="unsignedInt"/> - </attribute> + <optional> + <attribute name="port"> + <ref name="unsignedInt"/> + </attribute> + </optional> </group> <group> <attribute name="transport"> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index b801239..71da694 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -4013,11 +4013,6 @@ virDomainDiskDefParseXML(virCapsPtr caps, goto error; } hosts[nhosts - 1].port = virXMLPropString(child, "port"); - if (!hosts[nhosts - 1].port) { - virReportError(VIR_ERR_XML_ERROR, - "%s", _("missing port for host")); - goto error; - } } } child = child->next; diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 59773ec..5d52be5 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -2526,7 +2526,8 @@ qemuBuildDriveStr(virConnectPtr conn ATTRIBUTE_UNUSED, } else { /* only one host is supported now */ virBufferAsprintf(&opt, "file=sheepdog:%s:%s:", - disk->hosts->name, disk->hosts->port); + disk->hosts->name, + disk->hosts->port ? disk->hosts->port : "7000"); virBufferEscape(&opt, ',', ",", "%s,", disk->src); } break; -- 1.8.1.2
On Mon, Feb 25, 2013 at 06:44:29PM +0100, Paolo Bonzini wrote:
Only sheepdog actually required it in the code, and we can use 7000 as the default---the same value that QEMU uses for the simple "sheepdog:VOLUME" syntax. With this change, the schema can be fixed to allow no port.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- docs/formatdomain.html.in | 6 ++++++ docs/schemas/domaincommon.rng | 8 +++++--- src/conf/domain_conf.c | 5 ----- src/qemu/qemu_command.c | 3 ++- 4 files changed, 13 insertions(+), 9 deletions(-)
ACK Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- docs/formatsecret.html.in | 12 ++++++++++++ docs/schemas/secret.rng | 10 ++++++++++ include/libvirt/libvirt.h.in | 1 + src/conf/secret_conf.c | 22 +++++++++++++++++++++- src/conf/secret_conf.h | 1 + src/secret/secret_driver.c | 8 ++++++++ 6 files changed, 53 insertions(+), 1 deletion(-) diff --git a/docs/formatsecret.html.in b/docs/formatsecret.html.in index 01aff2d..c3c4a25 100644 --- a/docs/formatsecret.html.in +++ b/docs/formatsecret.html.in @@ -66,6 +66,18 @@ device</a>. <span class="since">Since 0.9.7</span>. </p> + <h3>Usage type "iscsi"</h3> + + <p> + This secret is associated with an iSCSI target for CHAP authentication. + The <code><usage type='iscsi'></code> element must contain + a single <code>target</code> element that specifies a usage name + for the secret. The iSCSI secret can then be used by UUID or by + this usage name via the <code><auth></code> element of + a <a href="domain.html#elementsDisks">disk + device</a>. <span class="since">Since 1.0.4</span>. + </p> + <h2><a name="example">Example</a></h2> <pre> diff --git a/docs/schemas/secret.rng b/docs/schemas/secret.rng index e49bd5a..d7b8f83 100644 --- a/docs/schemas/secret.rng +++ b/docs/schemas/secret.rng @@ -41,6 +41,7 @@ <choice> <ref name='usagevolume'/> <ref name='usageceph'/> + <ref name='usageiscsi'/> <!-- More choices later --> </choice> </element> @@ -67,4 +68,13 @@ </element> </define> + <define name='usageiscsi'> + <attribute name='type'> + <value>iscsi</value> + </attribute> + <element name='target'> + <ref name='genericName'/> + </element> + </define> + </grammar> diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index ad30cd0..3f98ebe 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -3641,6 +3641,7 @@ typedef enum { VIR_SECRET_USAGE_TYPE_NONE = 0, VIR_SECRET_USAGE_TYPE_VOLUME = 1, VIR_SECRET_USAGE_TYPE_CEPH = 2, + VIR_SECRET_USAGE_TYPE_ISCSI = 3, #ifdef VIR_ENUM_SENTINELS VIR_SECRET_USAGE_TYPE_LAST diff --git a/src/conf/secret_conf.c b/src/conf/secret_conf.c index 891af65..06b9bb2 100644 --- a/src/conf/secret_conf.c +++ b/src/conf/secret_conf.c @@ -36,7 +36,7 @@ #define VIR_FROM_THIS VIR_FROM_SECRET VIR_ENUM_IMPL(virSecretUsageType, VIR_SECRET_USAGE_TYPE_LAST, - "none", "volume", "ceph") + "none", "volume", "ceph", "iscsi") void virSecretDefFree(virSecretDefPtr def) @@ -57,6 +57,10 @@ virSecretDefFree(virSecretDefPtr def) VIR_FREE(def->usage.ceph); break; + case VIR_SECRET_USAGE_TYPE_ISCSI: + VIR_FREE(def->usage.target); + break; + default: VIR_ERROR(_("unexpected secret usage type %d"), def->usage_type); break; @@ -108,6 +112,15 @@ virSecretDefParseUsage(xmlXPathContextPtr ctxt, } break; + case VIR_SECRET_USAGE_TYPE_ISCSI: + def->usage.target = virXPathString("string(./usage/target)", ctxt); + if (!def->usage.target) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Ceph usage specified, but target is missing")); + return -1; + } + break; + default: virReportError(VIR_ERR_INTERNAL_ERROR, _("unexpected secret usage type %d"), @@ -262,6 +275,13 @@ virSecretDefFormatUsage(virBufferPtr buf, } break; + case VIR_SECRET_USAGE_TYPE_ISCSI: + if (def->usage.target != NULL) { + virBufferEscapeString(buf, " <target>%s</target>\n", + def->usage.target); + } + break; + default: virReportError(VIR_ERR_INTERNAL_ERROR, _("unexpected secret usage type %d"), diff --git a/src/conf/secret_conf.h b/src/conf/secret_conf.h index 6079d5b..53517f9 100644 --- a/src/conf/secret_conf.h +++ b/src/conf/secret_conf.h @@ -39,6 +39,7 @@ struct _virSecretDef { union { char *volume; /* May be NULL */ char *ceph; + char *target; } usage; }; diff --git a/src/secret/secret_driver.c b/src/secret/secret_driver.c index 5be33b9..c577817 100644 --- a/src/secret/secret_driver.c +++ b/src/secret/secret_driver.c @@ -149,6 +149,11 @@ secretFindByUsage(virSecretDriverStatePtr driver, int usageType, const char *usa if (STREQ(s->def->usage.ceph, usageID)) return s; break; + + case VIR_SECRET_USAGE_TYPE_ISCSI: + if (STREQ(s->def->usage.target, usageID)) + return s; + break; } } return NULL; @@ -614,6 +619,9 @@ secretUsageIDForDef(virSecretDefPtr def) case VIR_SECRET_USAGE_TYPE_CEPH: return def->usage.ceph; + case VIR_SECRET_USAGE_TYPE_ISCSI: + return def->usage.target; + default: return NULL; } -- 1.8.1.2
On Mon, Feb 25, 2013 at 06:44:30PM +0100, Paolo Bonzini wrote:
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- docs/formatsecret.html.in | 12 ++++++++++++ docs/schemas/secret.rng | 10 ++++++++++ include/libvirt/libvirt.h.in | 1 + src/conf/secret_conf.c | 22 +++++++++++++++++++++- src/conf/secret_conf.h | 1 + src/secret/secret_driver.c | 8 ++++++++ 6 files changed, 53 insertions(+), 1 deletion(-)
ACK Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- docs/formatdomain.html.in | 12 ++++----- docs/schemas/domaincommon.rng | 1 + src/conf/domain_conf.c | 31 ++++++++++++++++------ .../qemuxml2argv-disk-drive-network-iscsi-auth.xml | 31 ++++++++++++++++++++++ tests/qemuxml2xmltest.c | 1 + 5 files changed, 62 insertions(+), 14 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi-auth.xml diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index c590427..0906fe9 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -1760,12 +1760,12 @@ holds the actual password or other credentials (the domain XML intentionally does not expose the password, only the reference to the object that does manage the password). For now, the - only known secret <code>type</code> is "ceph", for Ceph RBD - network sources, and requires either an - attribute <code>uuid</code> with the UUID of the Ceph secret - object, or an attribute <code>usage</code> with the name - associated with the Ceph secret - object. <span class="since">libvirt 0.9.7</span> + known secret <code>type</code>s are "ceph", for Ceph RBD + network sources, and "iscsi", for CHAP authentication of iSCSI + targets. Both require either a <code>uuid</code> attribute + with the UUID of the secret object, or a <code>usage</code> + attribute matching the key that was specified in the + secret object. <span class="since">libvirt 0.9.7</span> </dd> <dt><code>geometry</code></dt> <dd>The optional <code>geometry</code> element provides the diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index b8c4503..6f85e84 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -3592,6 +3592,7 @@ <attribute name='type'> <choice> <value>ceph</value> + <value>iscsi</value> </choice> </attribute> <choice> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 71da694..e4c3e67 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -3885,6 +3885,8 @@ virDomainDiskDefParseXML(virCapsPtr caps, char *wwn = NULL; char *vendor = NULL; char *product = NULL; + int expected_secret_usage = -1; + int auth_secret_usage = -1; if (VIR_ALLOC(def) < 0) { virReportOOMError(); @@ -3922,7 +3924,6 @@ virDomainDiskDefParseXML(virCapsPtr caps, if (cur->type == XML_ELEMENT_NODE) { if (!source && !hosts && xmlStrEqual(cur->name, BAD_CAST "source")) { - sourceNode = cur; switch (def->type) { @@ -3958,6 +3959,9 @@ virDomainDiskDefParseXML(virCapsPtr caps, _("invalid logical unit number")); goto error; } + expected_secret_usage = VIR_SECRET_USAGE_TYPE_ISCSI; + } else if (def->protocol == VIR_DOMAIN_DISK_PROTOCOL_RBD) { + expected_secret_usage = VIR_SECRET_USAGE_TYPE_CEPH; } if (!(source = virXMLPropString(cur, "name")) && def->protocol != VIR_DOMAIN_DISK_PROTOCOL_NBD) { @@ -4144,8 +4148,9 @@ virDomainDiskDefParseXML(virCapsPtr caps, _("missing type for secret")); goto error; } - if (virSecretUsageTypeTypeFromString(usageType) != - VIR_SECRET_USAGE_TYPE_CEPH) { + auth_secret_usage = + virSecretUsageTypeTypeFromString(usageType); + if (auth_secret_usage < 0) { virReportError(VIR_ERR_XML_ERROR, _("invalid secret type %s"), usageType); @@ -4295,6 +4300,13 @@ virDomainDiskDefParseXML(virCapsPtr caps, cur = cur->next; } + if (auth_secret_usage != -1 && auth_secret_usage != expected_secret_usage) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("invalid secret type '%s'"), + virSecretUsageTypeTypeToString(auth_secret_usage)); + goto error; + } + device = virXMLPropString(node, "device"); if (device) { if ((def->device = virDomainDiskDeviceTypeFromString(device)) < 0) { @@ -12500,15 +12512,18 @@ virDomainDiskDefFormat(virBufferPtr buf, if (def->auth.username) { virBufferEscapeString(buf, " <auth username='%s'>\n", def->auth.username); + if (def->protocol == VIR_DOMAIN_DISK_PROTOCOL_ISCSI) { + virBufferAsprintf(buf, " <secret type='iscsi'"); + } else if (def->protocol == VIR_DOMAIN_DISK_PROTOCOL_RBD) { + virBufferAsprintf(buf, " <secret type='ceph'"); + } + if (def->auth.secretType == VIR_DOMAIN_DISK_SECRET_TYPE_UUID) { virUUIDFormat(def->auth.secret.uuid, uuidstr); - virBufferAsprintf(buf, - " <secret type='ceph' uuid='%s'/>\n", - uuidstr); + virBufferAsprintf(buf, " uuid='%s'/>\n", uuidstr); } if (def->auth.secretType == VIR_DOMAIN_DISK_SECRET_TYPE_USAGE) { - virBufferEscapeString(buf, - " <secret type='ceph' usage='%s'/>\n", + virBufferEscapeString(buf, " usage='%s'/>\n", def->auth.secret.usage); } virBufferAddLit(buf, " </auth>\n"); diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi-auth.xml b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi-auth.xml new file mode 100644 index 0000000..acaa503 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi-auth.xml @@ -0,0 +1,31 @@ +<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</emulator> + <disk type='network' device='disk'> + <driver name='qemu' type='raw'/> + <auth username='myname'> + <secret type='iscsi' usage='mycluster_myname'/> + </auth> + <source protocol='iscsi' name='iqn.1992-01.com.example'> + <host name='example.org'/> + </source> + <target dev='vda' bus='virtio'/> + </disk> + <controller type='usb' index='0'/> + <controller type='ide' index='0'/> + <memballoon model='virtio'/> + </devices> +</domain> diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index e0d3b20..076bf79 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -172,6 +172,7 @@ mymain(void) DO_TEST("disk-drive-network-nbd-ipv6-export"); DO_TEST("disk-drive-network-nbd-unix"); DO_TEST("disk-drive-network-iscsi"); + DO_TEST("disk-drive-network-iscsi-auth"); DO_TEST("disk-scsi-device"); DO_TEST("disk-scsi-vscsi"); DO_TEST("disk-scsi-virtio-scsi"); -- 1.8.1.2
On Mon, Feb 25, 2013 at 06:44:31PM +0100, Paolo Bonzini wrote:
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- docs/formatdomain.html.in | 12 ++++----- docs/schemas/domaincommon.rng | 1 + src/conf/domain_conf.c | 31 ++++++++++++++++------ .../qemuxml2argv-disk-drive-network-iscsi-auth.xml | 31 ++++++++++++++++++++++ tests/qemuxml2xmltest.c | 1 + 5 files changed, 62 insertions(+), 14 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi-auth.xml
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index c590427..0906fe9 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -1760,12 +1760,12 @@ holds the actual password or other credentials (the domain XML intentionally does not expose the password, only the reference to the object that does manage the password). For now, the - only known secret <code>type</code> is "ceph", for Ceph RBD - network sources, and requires either an - attribute <code>uuid</code> with the UUID of the Ceph secret - object, or an attribute <code>usage</code> with the name - associated with the Ceph secret - object. <span class="since">libvirt 0.9.7</span> + known secret <code>type</code>s are "ceph", for Ceph RBD + network sources, and "iscsi", for CHAP authentication of iSCSI + targets. Both require either a <code>uuid</code> attribute + with the UUID of the secret object, or a <code>usage</code> + attribute matching the key that was specified in the + secret object. <span class="since">libvirt 0.9.7</span> </dd> <dt><code>geometry</code></dt> <dd>The optional <code>geometry</code> element provides the diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index b8c4503..6f85e84 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -3592,6 +3592,7 @@ <attribute name='type'> <choice> <value>ceph</value> + <value>iscsi</value> </choice> </attribute> <choice> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 71da694..e4c3e67 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -3885,6 +3885,8 @@ virDomainDiskDefParseXML(virCapsPtr caps, char *wwn = NULL; char *vendor = NULL; char *product = NULL; + int expected_secret_usage = -1; + int auth_secret_usage = -1;
if (VIR_ALLOC(def) < 0) { virReportOOMError(); @@ -3922,7 +3924,6 @@ virDomainDiskDefParseXML(virCapsPtr caps, if (cur->type == XML_ELEMENT_NODE) { if (!source && !hosts && xmlStrEqual(cur->name, BAD_CAST "source")) { - sourceNode = cur;
switch (def->type) { @@ -3958,6 +3959,9 @@ virDomainDiskDefParseXML(virCapsPtr caps, _("invalid logical unit number")); goto error; } + expected_secret_usage = VIR_SECRET_USAGE_TYPE_ISCSI; + } else if (def->protocol == VIR_DOMAIN_DISK_PROTOCOL_RBD) { + expected_secret_usage = VIR_SECRET_USAGE_TYPE_CEPH; } if (!(source = virXMLPropString(cur, "name")) && def->protocol != VIR_DOMAIN_DISK_PROTOCOL_NBD) { @@ -4144,8 +4148,9 @@ virDomainDiskDefParseXML(virCapsPtr caps, _("missing type for secret")); goto error; } - if (virSecretUsageTypeTypeFromString(usageType) != - VIR_SECRET_USAGE_TYPE_CEPH) { + auth_secret_usage = + virSecretUsageTypeTypeFromString(usageType); + if (auth_secret_usage < 0) { virReportError(VIR_ERR_XML_ERROR, _("invalid secret type %s"), usageType); @@ -4295,6 +4300,13 @@ virDomainDiskDefParseXML(virCapsPtr caps, cur = cur->next; }
+ if (auth_secret_usage != -1 && auth_secret_usage != expected_secret_usage) { + virReportError(VIR_ERR_INTERNAL_ERROR,
Probably should use VIR_ERR_CONFIG_UNSUPPORTED
+ _("invalid secret type '%s'"), + virSecretUsageTypeTypeToString(auth_secret_usage));
And list the expected type here, as well as the incorrect type. ACK if those small changes are made when pushing Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
A better way to do this would be to use a configuration file like [iscsi "target-name"] user = name password = pwd and pass it via -readconfig. This would remove the username and password from the "ps" output. For now, however, keep this solution. Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- src/qemu/qemu_command.c | 80 ++++++++++++++++++---- ...qemuxml2argv-disk-drive-network-iscsi-auth.args | 1 + tests/qemuxml2argvtest.c | 2 + 3 files changed, 70 insertions(+), 13 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi-auth.args diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 5d52be5..30ddbd3 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -1888,8 +1888,8 @@ qemuBuildRBDString(virConnectPtr conn, VIR_FREE(base64); } else { virReportError(VIR_ERR_INTERNAL_ERROR, - _("rbd username '%s' specified but secret not found"), - disk->auth.username); + _("%s username '%s' specified but secret not found"), + "rbd", disk->auth.username); goto error; } } else { @@ -2057,6 +2057,7 @@ qemuParseDriveURIString(virDomainDiskDefPtr def, virURIPtr uri, char *transp = NULL; char *sock = NULL; char *volimg = NULL; + char *secret = NULL; if (VIR_ALLOC(def->hosts) < 0) goto no_memory; @@ -2117,6 +2118,16 @@ qemuParseDriveURIString(virDomainDiskDefPtr def, virURIPtr uri, def->src = NULL; } + if (uri->user) { + secret = strchr(uri->user, ':'); + if (secret) + *secret = '\0'; + + def->auth.username = strdup(uri->user); + if (!def->auth.username) + goto no_memory; + } + def->nhosts = 1; ret = 0; @@ -2237,14 +2248,20 @@ error: } static int -qemuBuildDriveURIString(virDomainDiskDefPtr disk, virBufferPtr opt, - const char *scheme) +qemuBuildDriveURIString(virConnectPtr conn, + virDomainDiskDefPtr disk, virBufferPtr opt, + const char *scheme, virSecretUsageType secretType) { int ret = -1; int port = 0; + virSecretPtr sec = NULL; + char *secret = NULL; + size_t secret_size; + char *tmpscheme = NULL; char *volimg = NULL; char *sock = NULL; + char *user = NULL; char *builturi = NULL; const char *transp = NULL; virURI uri = { @@ -2280,8 +2297,42 @@ qemuBuildDriveURIString(virDomainDiskDefPtr disk, virBufferPtr opt, virAsprintf(&sock, "socket=%s", disk->hosts->socket) < 0) goto no_memory; + if (disk->auth.username && secretType != VIR_SECRET_USAGE_TYPE_NONE) { + /* look up secret */ + switch (disk->auth.secretType) { + case VIR_DOMAIN_DISK_SECRET_TYPE_UUID: + sec = virSecretLookupByUUID(conn, + disk->auth.secret.uuid); + break; + case VIR_DOMAIN_DISK_SECRET_TYPE_USAGE: + sec = virSecretLookupByUsage(conn, secretType, + disk->auth.secret.usage); + break; + } + + if (sec) { + secret = (char *)conn->secretDriver->getValue(sec, &secret_size, 0, + VIR_SECRET_GET_VALUE_INTERNAL_CALL); + if (secret == NULL) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("could not get the value of the secret for username %s"), + disk->auth.username); + ret = -1; + goto cleanup; + } + if (virAsprintf(&user, "%s:%s", disk->auth.username, secret) < 0) + goto no_memory; + } else { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("%s username '%s' specified but secret not found"), + scheme, disk->auth.username); + ret = -1; + goto cleanup; + } + } uri.scheme = tmpscheme; /* gluster+<transport> */ uri.server = disk->hosts->name; + uri.user = user; uri.port = port; uri.path = volimg; uri.query = sock; @@ -2305,16 +2356,18 @@ no_memory: } static int -qemuBuildGlusterString(virDomainDiskDefPtr disk, virBufferPtr opt) +qemuBuildGlusterString(virConnectPtr conn, virDomainDiskDefPtr disk, virBufferPtr opt) { - return qemuBuildDriveURIString(disk, opt, "gluster"); + return qemuBuildDriveURIString(conn, disk, opt, "gluster", + VIR_SECRET_USAGE_TYPE_NONE); } static int -qemuBuildISCSIString(virDomainDiskDefPtr disk, virBufferPtr opt) +qemuBuildISCSIString(virConnectPtr conn, virDomainDiskDefPtr disk, virBufferPtr opt) { int ret; - ret = qemuBuildDriveURIString(disk, opt, "iscsi"); + ret = qemuBuildDriveURIString(conn, disk, opt, "iscsi", + VIR_SECRET_USAGE_TYPE_ISCSI); if (ret < 0) return ret; @@ -2325,7 +2378,7 @@ qemuBuildISCSIString(virDomainDiskDefPtr disk, virBufferPtr opt) } static int -qemuBuildNBDString(virDomainDiskDefPtr disk, virBufferPtr opt) +qemuBuildNBDString(virConnectPtr conn, virDomainDiskDefPtr disk, virBufferPtr opt) { const char *transp; @@ -2340,7 +2393,8 @@ qemuBuildNBDString(virDomainDiskDefPtr disk, virBufferPtr opt) && !disk->hosts->name) || (disk->hosts->transport == VIR_DOMAIN_DISK_PROTO_TRANS_UNIX && disk->hosts->socket && disk->hosts->socket[0] != '/')) - return qemuBuildDriveURIString(disk, opt, "nbd"); + return qemuBuildDriveURIString(conn, disk, opt, "nbd", + VIR_SECRET_USAGE_TYPE_NONE); virBufferAddLit(opt, "file=nbd:"); @@ -2498,7 +2552,7 @@ qemuBuildDriveStr(virConnectPtr conn ATTRIBUTE_UNUSED, } else if (disk->type == VIR_DOMAIN_DISK_TYPE_NETWORK) { switch (disk->protocol) { case VIR_DOMAIN_DISK_PROTOCOL_NBD: - if (qemuBuildNBDString(disk, &opt) < 0) + if (qemuBuildNBDString(conn, disk, &opt) < 0) goto error; virBufferAddChar(&opt, ','); break; @@ -2509,12 +2563,12 @@ qemuBuildDriveStr(virConnectPtr conn ATTRIBUTE_UNUSED, virBufferAddChar(&opt, ','); break; case VIR_DOMAIN_DISK_PROTOCOL_GLUSTER: - if (qemuBuildGlusterString(disk, &opt) < 0) + if (qemuBuildGlusterString(conn, disk, &opt) < 0) goto error; virBufferAddChar(&opt, ','); break; case VIR_DOMAIN_DISK_PROTOCOL_ISCSI: - if (qemuBuildISCSIString(disk, &opt) < 0) + if (qemuBuildISCSIString(conn, disk, &opt) < 0) goto error; virBufferAddChar(&opt, ','); break; diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi-auth.args b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi-auth.args new file mode 100644 index 0000000..fd2660a --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi-auth.args @@ -0,0 +1 @@ +LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test /usr/bin/qemu -S -M pc -m 214 -smp 1 -nographic -monitor unix:/tmp/test-monitor,server,nowait -no-acpi -boot c -usb -drive file=iscsi://myname:AQCVn5hO6HzFAhAAq0NCv8jtJcIcE+HOBlMQ1A@example.org/iqn.1992-01.com.example,if=virtio,format=raw -net none -serial none -parallel none diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index babdd8c..0afecf3 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -495,6 +495,8 @@ mymain(void) QEMU_CAPS_DRIVE, QEMU_CAPS_DRIVE_FORMAT); DO_TEST("disk-drive-network-iscsi", QEMU_CAPS_DRIVE, QEMU_CAPS_DRIVE_FORMAT); + DO_TEST("disk-drive-network-iscsi-auth", + QEMU_CAPS_DRIVE, QEMU_CAPS_DRIVE_FORMAT); DO_TEST("disk-drive-network-gluster", QEMU_CAPS_DRIVE, QEMU_CAPS_DRIVE_FORMAT); DO_TEST("disk-drive-network-rbd", -- 1.8.1.2
On Mon, Feb 25, 2013 at 06:44:32PM +0100, Paolo Bonzini wrote:
A better way to do this would be to use a configuration file like
[iscsi "target-name"] user = name password = pwd
and pass it via -readconfig. This would remove the username and password from the "ps" output. For now, however, keep this solution.
Urgh, we only accepted this for RBD on the condition that they were working on adding a monitor command to set secrets securely. Did that never happen ? /me wishes we'd never added code to allow it to be passed the CLI.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- src/qemu/qemu_command.c | 80 ++++++++++++++++++---- ...qemuxml2argv-disk-drive-network-iscsi-auth.args | 1 + tests/qemuxml2argvtest.c | 2 + 3 files changed, 70 insertions(+), 13 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi-auth.args
Very reluctant ACK on the basis that its is no worse than the mess we already have for rbd :-( Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
This enables usage of commands like persistent reservations. Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- src/libvirt_private.syms | 1 + src/qemu/qemu_command.c | 9 ++++++- .../qemuxml2argv-disk-drive-network-iscsi-lun.args | 1 + .../qemuxml2argv-disk-drive-network-iscsi-lun.xml | 28 ++++++++++++++++++++++ tests/qemuxml2argvtest.c | 4 ++++ 5 files changed, 42 insertions(+), 1 deletion(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi-lun.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi-lun.xml diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index f399871..8acff1a 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -156,6 +156,7 @@ virDomainDiskIoTypeToString; virDomainDiskPathByName; virDomainDiskProtocolTransportTypeFromString; virDomainDiskProtocolTransportTypeToString; +virDomainDiskProtocolTypeToString; virDomainDiskRemove; virDomainDiskRemoveByName; virDomainDiskTypeFromString; diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 30ddbd3..22e9d81 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -2844,7 +2844,14 @@ qemuBuildDriveDevStr(virDomainDefPtr def, bus); goto error; } - if (disk->type != VIR_DOMAIN_DISK_TYPE_BLOCK) { + if (disk->type == VIR_DOMAIN_DISK_TYPE_NETWORK) { + if (disk->protocol != VIR_DOMAIN_DISK_PROTOCOL_ISCSI) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("disk device='lun' is not supported for protocol='%s'"), + virDomainDiskProtocolTypeToString(disk->protocol)); + goto error; + } + } else if (disk->type != VIR_DOMAIN_DISK_TYPE_BLOCK) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("disk device='lun' is not supported for type='%s'"), virDomainDiskTypeToString(disk->type)); diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi-lun.args b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi-lun.args new file mode 100644 index 0000000..baa7760 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi-lun.args @@ -0,0 +1 @@ +LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test /usr/bin/qemu -S -M pc -m 214 -smp 1 -nographic -nodefconfig -nodefaults -monitor unix:/tmp/test-monitor,server,nowait -no-acpi -boot c -device virtio-scsi-pci,id=scsi0,bus=pci.0,addr=0x3 -usb -drive file=iscsi://example.org:3260/iqn.1992-01.com.example,if=none,id=drive-scsi0-0-0-0,format=raw -device scsi-block,bus=scsi0.0,channel=0,scsi-id=0,lun=0,drive=drive-scsi0-0-0-0,id=scsi0-0-0-0 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi-lun.xml b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi-lun.xml new file mode 100644 index 0000000..b6d6aaa --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi-lun.xml @@ -0,0 +1,28 @@ +<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</emulator> + <disk type='network' device='lun'> + <driver name='qemu' type='raw'/> + <source protocol='iscsi' name='iqn.1992-01.com.example'> + <host name='example.org' port='3260'/> + </source> + <target dev='sda' bus='scsi'/> + </disk> + <controller type='usb' index='0'/> + <controller type='scsi' model='virtio-scsi'/> + <memballoon model='none'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 0afecf3..5e0d976 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -497,6 +497,10 @@ mymain(void) QEMU_CAPS_DRIVE, QEMU_CAPS_DRIVE_FORMAT); DO_TEST("disk-drive-network-iscsi-auth", QEMU_CAPS_DRIVE, QEMU_CAPS_DRIVE_FORMAT); + DO_TEST("disk-drive-network-iscsi-lun", + QEMU_CAPS_DRIVE, QEMU_CAPS_DEVICE, QEMU_CAPS_DRIVE_FORMAT, + QEMU_CAPS_NODEFCONFIG, QEMU_CAPS_VIRTIO_SCSI_PCI, + QEMU_CAPS_VIRTIO_BLK_SG_IO, QEMU_CAPS_SCSI_BLOCK); DO_TEST("disk-drive-network-gluster", QEMU_CAPS_DRIVE, QEMU_CAPS_DRIVE_FORMAT); DO_TEST("disk-drive-network-rbd", -- 1.8.1.2
On Mon, Feb 25, 2013 at 07:09:14PM +0100, Paolo Bonzini wrote:
This enables usage of commands like persistent reservations.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- src/libvirt_private.syms | 1 + src/qemu/qemu_command.c | 9 ++++++- .../qemuxml2argv-disk-drive-network-iscsi-lun.args | 1 + .../qemuxml2argv-disk-drive-network-iscsi-lun.xml | 28 ++++++++++++++++++++++ tests/qemuxml2argvtest.c | 4 ++++ 5 files changed, 42 insertions(+), 1 deletion(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi-lun.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi-lun.xml
ACK Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 02/25/2013 10:44 AM, Paolo Bonzini wrote:
This series improves support for NBD disks (patches 1-6), and adds support for the libiscsi userspace initiator (patches 7-13).
1 is a definite bug fix, and deserves to be in 1.0.3. It's too late for me tonight to do any more reviewing, but I also hope to check tomorrow whether any of 2-6 also make sense during the freeze. Patches 7-13 should wait until after 1.0.3 is out, since they are definitely new material and not bug fixes. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Il 28/02/2013 06:10, Eric Blake ha scritto:
It's too late for me tonight to do any more reviewing, but I also hope to check tomorrow whether any of 2-6 also make sense during the freeze. Patches 7-13 should wait until after 1.0.3 is out, since they are definitely new material and not bug fixes.
No, I don't think any of 2-13 belong in 1.0.3. Paolo
Il 28/02/2013 06:10, Eric Blake ha scritto:
On 02/25/2013 10:44 AM, Paolo Bonzini wrote:
This series improves support for NBD disks (patches 1-6), and adds support for the libiscsi userspace initiator (patches 7-13).
1 is a definite bug fix, and deserves to be in 1.0.3. It's too late for me tonight to do any more reviewing, but I also hope to check tomorrow whether any of 2-6 also make sense during the freeze. Patches 7-13 should wait until after 1.0.3 is out, since they are definitely new material and not bug fixes.
1.0.3 is now out, ping! :) Paolo
participants (4)
-
Daniel P. Berrange -
Eric Blake -
Osier Yang -
Paolo Bonzini