[PATCH 0/7] Various fixes for Ceph drives and parsing libvirt XML.
Miscellaneous fixes to: - Handling of Ceph drives now works end-to-end (RHBZ#1026688). - In particular, you can now use rbd:/// URIs in guestfish (and they work). - Parse Ceph & NBD network drives from libvirt XML correctly, so that existing domains with Ceph/NBD drives can be added (eg. using guestfish -d option). - Add more testing of the above.
--- fish/uri.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/fish/uri.c b/fish/uri.c index 4c235e3..876d731 100644 --- a/fish/uri.c +++ b/fish/uri.c @@ -142,7 +142,7 @@ parse (const char *arg, char **path_ret, char **protocol_ret, *protocol_ret = strdup (uri->scheme); if (*protocol_ret == NULL) { - perror ("strdup"); + perror ("strdup: protocol"); return -1; } @@ -154,7 +154,7 @@ parse (const char *arg, char **path_ret, char **protocol_ret, if (uri->user && STRNEQ (uri->user, "")) { *username_ret = strdup (uri->user); if (*username_ret == NULL) { - perror ("username"); + perror ("strdup: username"); free (*protocol_ret); guestfs___free_string_list (*server_ret); return -1; -- 1.8.4.2
The path at the protocol level is: pool/disk (with no leading '/' character). This is now what you have to pass to guestfs_add_drive_opts. Also Ceph can be called with no explicit servers (it uses the contents of /etc/ceph/ceph.conf instead). So allow zero servers to be used. --- generator/actions.ml | 2 +- src/drives.c | 9 ++------- src/guestfs.pod | 2 +- src/launch-direct.c | 10 +++++----- tests/disks/test-qemu-drive.sh | 10 +++++++++- 5 files changed, 18 insertions(+), 15 deletions(-) diff --git a/generator/actions.ml b/generator/actions.ml index d331988..fa1a2c5 100644 --- a/generator/actions.ml +++ b/generator/actions.ml @@ -1440,7 +1440,7 @@ is a list of server(s). gluster Exactly one iscsi Exactly one nbd Exactly one - rbd One or more + rbd Zero or more sheepdog Zero or more ssh Exactly one diff --git a/src/drives.c b/src/drives.c index 4f8a7c5..9646b08 100644 --- a/src/drives.c +++ b/src/drives.c @@ -323,11 +323,6 @@ create_drive_rbd (guestfs_h *g, { size_t i; - if (nr_servers == 0) { - error (g, _("rbd: you must specify one or more servers")); - return NULL; - } - for (i = 0; i < nr_servers; ++i) { if (servers[i].transport != drive_transport_none && servers[i].transport != drive_transport_tcp) { @@ -345,8 +340,8 @@ create_drive_rbd (guestfs_h *g, return NULL; } - if (exportname[0] != '/') { - error (g, _("rbd: image name must begin with a '/'")); + if (exportname[0] == '/') { + error (g, _("rbd: image name must not begin with a '/'")); return NULL; } diff --git a/src/guestfs.pod b/src/guestfs.pod index 1f0a324..292c97e 100644 --- a/src/guestfs.pod +++ b/src/guestfs.pod @@ -684,7 +684,7 @@ To do this, set the optional C<protocol> and C<server> parameters of L</guestfs_add_drive_opts> like this: char **servers = { "ceph1.example.org:3000", /* ... */, NULL }; - guestfs_add_drive_opts (g, "/pool/image", + guestfs_add_drive_opts (g, "pool/image", GUESTFS_ADD_DRIVE_OPTS_FORMAT, "raw", GUESTFS_ADD_DRIVE_OPTS_PROTOCOL, "rbd", GUESTFS_ADD_DRIVE_OPTS_SERVER, servers, diff --git a/src/launch-direct.c b/src/launch-direct.c index c2a6885..6eaee63 100644 --- a/src/launch-direct.c +++ b/src/launch-direct.c @@ -1299,12 +1299,12 @@ guestfs___drive_source_qemu_param (guestfs_h *g, const struct drive_source *src) } case drive_protocol_rbd: { - /* build the list of all the mon hosts */ CLEANUP_FREE char *mon_host = NULL, *username = NULL, *secret = NULL; const char *auth; size_t n = 0; size_t i, j; + /* build the list of all the mon hosts */ for (i = 0; i < src->nr_servers; i++) { n += strlen (src->servers[i].u.hostname); n += 8; /* for slashes, colons, & port numbers */ @@ -1340,10 +1340,10 @@ guestfs___drive_source_qemu_param (guestfs_h *g, const struct drive_source *src) else auth = ":auth_supported=none"; - /* Skip the mandatory leading '/' character on exportname. */ - return safe_asprintf (g, "rbd:%s:mon_host=%s%s%s%s", - &src->u.exportname[1], - mon_host, + return safe_asprintf (g, "rbd:%s%s%s%s%s%s", + src->u.exportname, + src->nr_servers > 0 ? ":mon_host=" : "", + src->nr_servers > 0 ? mon_host : "", username ? username : "", auth, secret ? secret : ""); diff --git a/tests/disks/test-qemu-drive.sh b/tests/disks/test-qemu-drive.sh index 3709c95..627d8e0 100755 --- a/tests/disks/test-qemu-drive.sh +++ b/tests/disks/test-qemu-drive.sh @@ -47,7 +47,7 @@ rm -f "$DEBUG_QEMU_FILE" # Ceph (RBD). $guestfish <<EOF ||: - add "/abc-def/ghi-jkl" "format:raw" "protocol:rbd" \ + add "abc-def/ghi-jkl" "format:raw" "protocol:rbd" \ "server:1.2.3.4:1234 1.2.3.5:1235 1.2.3.6:1236" run EOF @@ -55,6 +55,14 @@ check_output grep -sq -- '-drive file=rbd:abc-def/ghi-jkl:mon_host=1.2.3.4\\:1234\\;1.2.3.5\\:1235\\;1.2.3.6\\:1236:auth_supported=none,' "$DEBUG_QEMU_FILE" || fail rm "$DEBUG_QEMU_FILE" +$guestfish <<EOF ||: + add "abc-def/ghi-jkl" "format:raw" "protocol:rbd" + run +EOF +check_output +grep -sq -- '-drive file=rbd:abc-def/ghi-jkl:auth_supported=none,' "$DEBUG_QEMU_FILE" || fail +rm "$DEBUG_QEMU_FILE" + # HTTP. $guestfish <<EOF ||: -- 1.8.4.2
The path at the API level (for guestfs_add_drive_opts) is: pool/disk The URI syntax is either rbd:///pool/disk or rbd://server:port/pool/disk. Because of the way URI parsing works we may need to remove a leading '/' character before passing the path down to the API. --- fish/guestfish.pod | 6 ++++-- fish/test-add-uri.sh | 6 ++++-- fish/uri.c | 16 +++++++++++++--- 3 files changed, 21 insertions(+), 7 deletions(-) diff --git a/fish/guestfish.pod b/fish/guestfish.pod index 04d9aa9..0d81ba8 100644 --- a/fish/guestfish.pod +++ b/fish/guestfish.pod @@ -1198,7 +1198,9 @@ The equivalent API command would be (no export name):
<fs> add "" protocol:nbd server:[tcp:example.com|unix:/socket]
-=head2 B<-a rbd://example.com[:port]/disk> +=head2 B<-a rbd:///pool/disk> + +=head2 B<-a rbd://example.com[:port]/pool/disk> Add a disk image located on a Ceph (RBD/librbd) storage volume. @@ -1207,7 +1209,7 @@ server can be specified when using this URI syntax. The equivalent API command would be: - ><fs> add /disk protocol:rbd server:tcp:example.com + ><fs> add pool/disk protocol:rbd server:tcp:example.com:port =head2 B<-a sheepdog://[example.com[:port]]/volume/image> diff --git a/fish/test-add-uri.sh b/fish/test-add-uri.sh index dfeccf7..3d414b9 100755 --- a/fish/test-add-uri.sh +++ b/fish/test-add-uri.sh @@ -59,8 +59,10 @@ $VG ./guestfish -x -a 'nbd:///export?socket=/sk' </dev/null >test-add-uri.out 2> grep -sq 'add_drive "/export" "protocol:nbd" "server:unix:/sk"' test-add-uri.out || fail # rbd -$VG ./guestfish -x -a rbd://example.com:3000/disk </dev/null >test-add-uri.out 2>&1 -grep -sq 'add_drive "/disk" "protocol:rbd" "server:tcp:example.com:3000"' test-add-uri.out || fail +$VG ./guestfish -x -a rbd://example.com:6789/pool/disk </dev/null >test-add-uri.out 2>&1 +grep -sq 'add_drive "pool/disk" "protocol:rbd" "server:tcp:example.com:6789"' test-add-uri.out || fail +$VG ./guestfish -x -a rbd:///pool/disk </dev/null >test-add-uri.out 2>&1 +grep -sq 'add_drive "pool/disk" "protocol:rbd" test-add-uri.out || fail # sheepdog $VG ./guestfish -x -a sheepdog:///volume/image </dev/null >test-add-uri.out 2>&1 diff --git a/fish/uri.c b/fish/uri.c index 876d731..0d530aa 100644 --- a/fish/uri.c +++ b/fish/uri.c @@ -105,6 +105,7 @@ parse (const char *arg, char **path_ret, char **protocol_ret, { CLEANUP_XMLFREEURI xmlURIPtr uri = NULL; CLEANUP_FREE char *socket = NULL; + char *path; uri = xmlParseURI (arg); if (!uri) { @@ -162,9 +163,18 @@ parse (const char *arg, char **path_ret, char **protocol_ret, } else *username_ret = NULL; - *path_ret = strdup (uri->path ? uri->path : ""); - if (!*path_ret) { - perror ("path"); + /* We may have to adjust the path depending on the protocol. For + * example ceph/rbd URIs look like rbd:///pool/disk, but the + * exportname expected will be "pool/disk". Here, uri->path will be + * "/pool/disk" so we have to knock off the leading '/' character. + */ + path = uri->path; + if (STREQ (uri->scheme, "rbd") && path[0] == '/') + path++; + + *path_ret = strdup (path ? path : ""); + if (*path_ret == NULL) { + perror ("strdup: path"); free (*protocol_ret); guestfs___free_string_list (*server_ret); free (*username_ret); -- 1.8.4.2
This is valid for some network drives: <source protocol='rbd' name='abc-def/ghi-jkl'/> instead of this: <source protocol='rbd' name='abc-def/ghi-jkl'> <host name='foo' port='1234'/> </source> Allow both forms. --- src/libvirt-domain.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c index ace2e00..af9770f 100644 --- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c @@ -541,8 +541,7 @@ for_each_disk (guestfs_h *g, xphost = xmlXPathEvalExpression (BAD_CAST "./source/host", xpathCtx); if (xphost == NULL || - xphost->nodesetval == NULL || - xphost->nodesetval->nodeNr == 0) + xphost->nodesetval == NULL) continue; /* This gives us a list of <host> elements, which each have a -- 1.8.4.2
--- src/libvirt-domain.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c index af9770f..a041e8f 100644 --- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c @@ -489,18 +489,16 @@ for_each_disk (guestfs_h *g, xpathCtx); if (xpfilename == NULL || xpfilename->nodesetval == NULL || - xpfilename->nodesetval->nodeNr == 0) { + xpfilename->nodesetval->nodeNr == 0) continue; /* disk filename not found, skip this */ - } } else if (STREQ (type, "block")) { /* type = "block", use source/@dev */ xpathCtx->node = nodes->nodeTab[i]; xpfilename = xmlXPathEvalExpression (BAD_CAST "./source/@dev", xpathCtx); if (xpfilename == NULL || xpfilename->nodesetval == NULL || - xpfilename->nodesetval->nodeNr == 0) { + xpfilename->nodesetval->nodeNr == 0) continue; /* disk filename not found, skip this */ - } } else if (STREQ (type, "network")) { /* type = "network", use source/@name */ int hi; -- 1.8.4.2
This is valid for some network drives, for example: <source protocol='nbd'> <host name='foo' port='1234'/> </source> We pass an empty string as path to the guestfs_add_drive_opts API in this case. --- src/libvirt-domain.c | 37 +++++++++++++++++++++++-------------- 1 file changed, 23 insertions(+), 14 deletions(-) diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c index a041e8f..414e996 100644 --- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c @@ -504,26 +504,29 @@ for_each_disk (guestfs_h *g, debug (g, _("disk[%zu]: network device"), i); xpathCtx->node = nodes->nodeTab[i]; - xpfilename = xmlXPathEvalExpression (BAD_CAST "./source/@name", - xpathCtx); - if (xpfilename == NULL || - xpfilename->nodesetval == NULL || - xpfilename->nodesetval->nodeNr == 0) - continue; + /* Get the protocol (e.g. "rbd"). Required. */ xpprotocol = xmlXPathEvalExpression (BAD_CAST "./source/@protocol", xpathCtx); - /* Get the protocol (e.g. "rbd"). */ if (xpprotocol == NULL || xpprotocol->nodesetval == NULL || xpprotocol->nodesetval->nodeNr == 0) continue; assert (xpprotocol->nodesetval->nodeTab[0]); - assert (xpprotocol->nodesetval->nodeTab[0]->type == XML_ATTRIBUTE_NODE); + assert (xpprotocol->nodesetval->nodeTab[0]->type == + XML_ATTRIBUTE_NODE); attr = (xmlAttrPtr) xpprotocol->nodesetval->nodeTab[0]; protocol = (char *) xmlNodeListGetString (doc, attr->children, 1); debug (g, _("disk[%zu]: protocol: %s"), i, protocol); + /* <source name="..."> is the path/exportname. Optional. */ + xpfilename = xmlXPathEvalExpression (BAD_CAST "./source/@name", + xpathCtx); + if (xpfilename == NULL || + xpfilename->nodesetval == NULL) + continue; + + /* <auth username="...">. Optional. */ xpusername = xmlXPathEvalExpression (BAD_CAST "./auth/@username", xpathCtx); if (xpusername != NULL && @@ -571,15 +574,21 @@ for_each_disk (guestfs_h *g, * ./auth/secret/@usage || ./auth/secret/@uuid */ } else - continue; /* type <> "file", "block", or "network", skip it */ + continue; /* type <> "file", "block", or "network", skip it */ assert (xpfilename); assert (xpfilename->nodesetval); - assert (xpfilename->nodesetval->nodeTab[0]); - assert (xpfilename->nodesetval->nodeTab[0]->type == XML_ATTRIBUTE_NODE); - attr = (xmlAttrPtr) xpfilename->nodesetval->nodeTab[0]; - filename = (char *) xmlNodeListGetString (doc, attr->children, 1); - debug (g, _("disk[%zu]: filename: %s"), i, filename); + if (xpfilename->nodesetval->nodeNr > 0) { + assert (xpfilename->nodesetval->nodeTab[0]); + assert (xpfilename->nodesetval->nodeTab[0]->type == + XML_ATTRIBUTE_NODE); + attr = (xmlAttrPtr) xpfilename->nodesetval->nodeTab[0]; + filename = (char *) xmlNodeListGetString (doc, attr->children, 1); + debug (g, _("disk[%zu]: filename: %s"), i, filename); + } + else + /* For network protocols (eg. nbd), name may be omitted. */ + filename = safe_strdup (g, ""); /* Get the disk format (may not be set). */ xpathCtx->node = nodes->nodeTab[i]; -- 1.8.4.2
This tests that libvirt domain XML (ie. guestfish -d option) is parsed correctly and results in the correct qemu command line when using the direct backend. This is a good end-to-end test of various layers. --- .gitignore | 1 + configure.ac | 1 + tests/disks/Makefile.am | 6 ++- tests/disks/test-qemu-drive-libvirt.sh | 82 ++++++++++++++++++++++++++++++ tests/disks/test-qemu-drive-libvirt.xml.in | 80 +++++++++++++++++++++++++++++ 5 files changed, 169 insertions(+), 1 deletion(-) create mode 100755 tests/disks/test-qemu-drive-libvirt.sh create mode 100644 tests/disks/test-qemu-drive-libvirt.xml.in diff --git a/.gitignore b/.gitignore index 26d5549..f8e6c71 100644 --- a/.gitignore +++ b/.gitignore @@ -468,6 +468,7 @@ Makefile.in /tests/data/initrd-x86_64.img.gz /tests/data/test-grep.txt.gz /tests/data/test.iso +/tests/disks/test-qemu-drive-libvirt.xml /tests/events/test-libvirt-auth-callbacks /tests/guests/blank-*.img /tests/guests/debian.img diff --git a/configure.ac b/configure.ac index 3d7f034..15ef432 100644 --- a/configure.ac +++ b/configure.ac @@ -1747,6 +1747,7 @@ AC_CONFIG_FILES([Makefile tests/charsets/Makefile tests/data/Makefile tests/disks/Makefile + tests/disks/test-qemu-drive-libvirt.xml tests/disk-labels/Makefile tests/events/Makefile tests/fuzz/Makefile diff --git a/tests/disks/Makefile.am b/tests/disks/Makefile.am index 9aff5e8..1315e71 100644 --- a/tests/disks/Makefile.am +++ b/tests/disks/Makefile.am @@ -19,10 +19,14 @@ include $(top_srcdir)/subdir-rules.mk TESTS = \ test-max-disks.pl \ + test-qemu-drive-libvirt.sh \ test-qemu-drive.sh -TESTS_ENVIRONMENT = $(top_builddir)/run --test +TESTS_ENVIRONMENT = \ + abs_srcdir=$(abs_srcdir) \ + $(top_builddir)/run --test EXTRA_DIST = \ debug-qemu.sh \ + test-qemu-drive-libvirt.xml \ $(TESTS) diff --git a/tests/disks/test-qemu-drive-libvirt.sh b/tests/disks/test-qemu-drive-libvirt.sh new file mode 100755 index 0000000..dd0ca0e --- /dev/null +++ b/tests/disks/test-qemu-drive-libvirt.sh @@ -0,0 +1,82 @@ +#!/bin/bash +# Copyright (C) 2013-2014 Red Hat Inc. +# +# This program is free software; you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 2 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write to the Free Software +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. + +# Test that disks map to the correct qemu -drive parameter. + +export LANG=C + +set -e + +if [ -z "$abs_srcdir" ]; then + echo "$0: abs_srcdir environment variable must be set" + exit 1 +fi + +guestfish="\ + ../../fish/guestfish -c test://$abs_srcdir/test-qemu-drive-libvirt.xml" + +export LIBGUESTFS_BACKEND=direct +export LIBGUESTFS_HV="$(pwd)/debug-qemu.sh" +export DEBUG_QEMU_FILE="$(pwd)/test-qemu-drive-libvirt.out" + +function check_output () +{ + if [ ! -f "$DEBUG_QEMU_FILE" ]; then + echo "$0: guestfish command failed, see previous error messages" + exit 1 + fi +} + +function fail () +{ + echo "$0: Test failed. Command line output was:" + cat "$DEBUG_QEMU_FILE" + exit 1 +} + +rm -f "$DEBUG_QEMU_FILE" + +# Ceph (RBD). + +$guestfish -d ceph1 run ||: +check_output +grep -sq -- '-drive file=rbd:abc-def/ghi-jkl:mon_host=1.2.3.4\\:1234\\;1.2.3.5\\:1235\\;1.2.3.6\\:1236:auth_supported=none,' "$DEBUG_QEMU_FILE" || fail +rm "$DEBUG_QEMU_FILE" + +$guestfish -d ceph2 run ||: +check_output +grep -sq -- '-drive file=rbd:abc-def/ghi-jkl:auth_supported=none,' "$DEBUG_QEMU_FILE" || fail +rm "$DEBUG_QEMU_FILE" + +# NBD. + +$guestfish -d nbd run ||: +check_output +grep -sq -- '-drive file=nbd:1.2.3.4:1234,' "$DEBUG_QEMU_FILE" || fail +rm "$DEBUG_QEMU_FILE" + +# To do: + +# HTTP - curl not yet supported by libvirt + +# Gluster. + +# iSCSI. + +# Sheepdog. + +# SSH. diff --git a/tests/disks/test-qemu-drive-libvirt.xml.in b/tests/disks/test-qemu-drive-libvirt.xml.in new file mode 100644 index 0000000..dea739d --- /dev/null +++ b/tests/disks/test-qemu-drive-libvirt.xml.in @@ -0,0 +1,80 @@ +<!-- + @configure_input@ + Copyright (C) 2013-2014 Red Hat Inc. + + This program is free software; you can redistribute it and/or modify + it under the terms of the GNU General Public License as published by + the Free Software Foundation; either version 2 of the License, or + (at your option) any later version. + + This program is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + GNU General Public License for more details. + + You should have received a copy of the GNU General Public License + along with this program; if not, write to the Free Software + Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. + + Test XML for test-qemu-drive-libvirt.sh +--> +<node> + + <domain type='test' xmlns:test='http://libvirt.org/schemas/domain/test/1.0'> + <test:runstate>5</test:runstate> <!-- 5 == VIR_DOMAIN_SHUTOFF --> + <name>ceph1</name> + <memory>1048576</memory> + <os> + <type>hvm</type> + <boot dev='hd'/> + </os> + <devices> + <disk type='network' device='disk'> + <driver name='qemu'/> + <source protocol='rbd' name='abc-def/ghi-jkl'> + <host name='1.2.3.4' port='1234'/> + <host name='1.2.3.5' port='1235'/> + <host name='1.2.3.6' port='1236'/> + </source> + <target dev='vda' bus='virtio'/> + </disk> + </devices> + </domain> + + <domain type='test' xmlns:test='http://libvirt.org/schemas/domain/test/1.0'> + <test:runstate>5</test:runstate> <!-- 5 == VIR_DOMAIN_SHUTOFF --> + <name>ceph2</name> + <memory>1048576</memory> + <os> + <type>hvm</type> + <boot dev='hd'/> + </os> + <devices> + <disk type='network' device='disk'> + <driver name='qemu'/> + <source protocol='rbd' name='abc-def/ghi-jkl'/> + <target dev='vda' bus='virtio'/> + </disk> + </devices> + </domain> + + <domain type='test' xmlns:test='http://libvirt.org/schemas/domain/test/1.0'> + <test:runstate>5</test:runstate> <!-- 5 == VIR_DOMAIN_SHUTOFF --> + <name>nbd</name> + <memory>1048576</memory> + <os> + <type>hvm</type> + <boot dev='hd'/> + </os> + <devices> + <disk type='network' device='disk'> + <driver name='qemu'/> + <source protocol='nbd'> + <host name='1.2.3.4' port='1234'/> + </source> + <target dev='vda' bus='virtio'/> + </disk> + </devices> + </domain> + +</node> -- 1.8.4.2
participants (1)
-
Richard W.M. Jones