Re: [libvirt] [PATCH] storage backend: Add RBD (RADOS Block Device) support

On 05/21/2012 05:00 AM, Daniel P. Berrange wrote: I did not closely review this patch, assuming that Daniel's review covered any issues such as avoiding resource leaks. Now pushed, with the following tweaks to keep 'make syntax-check' happy, fix typos and grammar in the docs, and fix configuration/compilation when librbd libraries (ceph-devel on Fedora) are not available, and fix compilation with warnings enabled when ceph-devel is installed: diff --git i/configure.ac w/configure.ac index 669a1eb..06c6a4b 100644 --- i/configure.ac +++ w/configure.ac @@ -1981,21 +1981,21 @@ if test "$with_storage_mpath" = "check"; then fi AM_CONDITIONAL([WITH_STORAGE_MPATH], [test "$with_storage_mpath" = "yes"]) +LIBRBD_LIBS= if test "$with_storage_rbd" = "yes" || test "$with_storage_rbd" = "check"; then AC_CHECK_HEADER([rbd/librbd.h], [LIBRBD_FOUND=yes; break;]) - LIBRBD_LIBS="-lrbd -lrados -lcrypto" - if test "$LIBRBD_FOUND" = "yes"; then with_storage_rbd=yes - LIBS="$LIBS $LIBRBD_LIBS" + LIBRBD_LIBS="-lrbd -lrados -lcrypto" + AC_DEFINE_UNQUOTED([WITH_STORAGE_RBD], [1], + [whether RBD backend for storage driver is enabled]) else with_storage_rbd=no fi - - AC_DEFINE_UNQUOTED([WITH_STORAGE_RBD], 1, [wether RBD backend for storage driver is enabled]) fi AM_CONDITIONAL([WITH_STORAGE_RBD], [test "$with_storage_rbd" = "yes"]) +AC_SUBST([LIBRBD_LIBS]) LIBPARTED_CFLAGS= LIBPARTED_LIBS= diff --git i/docs/schemas/storagepool.rng w/docs/schemas/storagepool.rng index 7753493..75b6b51 100644 --- i/docs/schemas/storagepool.rng +++ w/docs/schemas/storagepool.rng @@ -485,7 +485,7 @@ <empty/> </element> </define> - + <define name='sourcerbd'> <element name='source'> <ref name='sourceinfoname'/> diff --git i/docs/storage.html.in w/docs/storage.html.in index 277ea36..b3484e8 100644 --- i/docs/storage.html.in +++ w/docs/storage.html.in @@ -496,23 +496,20 @@ <h2><a name="StorageBackendRBD">RBD pools</a></h2> <p> - This storage driver provides a pool which contains all RBD images in a RADOS pool. - <br /> - RBD (RADOS Block Device) is part of the Ceph distributed storage project. - <br/> - This backend <i>only</i> supports Qemu with RBD support. Kernel RBD which exposes - RBD devices as block devices in /dev is <i>not</i> supported. - <br/> - RBD images created with this storage backend can be accessed through kernel RBD if - configured manually, but this backend does not provide mapping these images. - <br/> - Images created with this backend can be attached to Qemu guests when Qemu is build - with RBD support (Since Qemu 0.14.0)</a>. - <br/> - The backend supports cephx authentication for communication with the Ceph cluster. - <br/> - Storing the cephx authentication key is done with the libvirt secret mechanism. - The UUID in the example pool input refers to the UUID of the stored secret. + This storage driver provides a pool which contains all RBD + images in a RADOS pool. RBD (RADOS Block Device) is part + of the Ceph distributed storage project.<br/> + This backend <i>only</i> supports Qemu with RBD support. Kernel RBD + which exposes RBD devices as block devices in /dev is <i>not</i> + supported. RBD images created with this storage backend + can be accessed through kernel RBD if configured manually, but + this backend does not provide mapping for these images.<br/> + Images created with this backend can be attached to Qemu guests + when Qemu is build with RBD support (Since Qemu 0.14.0). The + backend supports cephx authentication for communication with the + Ceph cluster. Storing the cephx authentication key is done with + the libvirt secret mechanism. The UUID in the example pool input + refers to the UUID of the stored secret. <span class="since">Since 0.9.13</span> </p> @@ -552,7 +549,11 @@ &lt;/volume&gt;</pre> <h3>Example disk attachement</h3> - <p>RBD images can be attached to Qemu guests when Qemu is build with RBD support. Information about attaching a RBD image to a guest can be found at <a href="formatdomain.html#elementsDisks">format domain</a> page.</p> + <p>RBD images can be attached to Qemu guests when Qemu is built + with RBD support. Information about attaching a RBD image to a + guest can be found + at <a href="formatdomain.html#elementsDisks">format domain</a> + page.</p> <h3>Valid pool format types</h3> <p> diff --git i/po/POTFILES.in w/po/POTFILES.in index cfa9d44..91216cb 100644 --- i/po/POTFILES.in +++ w/po/POTFILES.in @@ -104,6 +104,7 @@ src/storage/storage_backend_fs.c src/storage/storage_backend_iscsi.c src/storage/storage_backend_logical.c src/storage/storage_backend_mpath.c +src/storage/storage_backend_rbd.c src/storage/storage_backend_scsi.c src/storage/storage_driver.c src/test/test_driver.c diff --git i/src/Makefile.am w/src/Makefile.am index 413464b..e9621c1 100644 --- i/src/Makefile.am +++ w/src/Makefile.am @@ -1052,7 +1052,7 @@ endif if WITH_STORAGE_RBD libvirt_driver_storage_la_SOURCES += $(STORAGE_DRIVER_RBD_SOURCES) -libvirt_la_LIBADD += $(LIBRBD_LIBS) +libvirt_driver_storage_la_LIBADD += $(LIBRBD_LIBS) endif if WITH_NODE_DEVICES diff --git i/src/storage/storage_backend_rbd.c w/src/storage/storage_backend_rbd.c index 5319749..46bd892 100644 --- i/src/storage/storage_backend_rbd.c +++ w/src/storage/storage_backend_rbd.c @@ -55,6 +55,8 @@ static int virStorageBackendRBDOpenRADOSConn(virStorageBackendRBDStatePtr *ptr, virBuffer mon_host = VIR_BUFFER_INITIALIZER; virSecretPtr secret = NULL; char secretUuid[VIR_UUID_STRING_BUFLEN]; + int i; + char *mon_buff = NULL; VIR_DEBUG("Found Cephx username: %s", pool->def->source.auth.cephx.username); @@ -130,9 +132,8 @@ static int virStorageBackendRBDOpenRADOSConn(virStorageBackendRBDStatePtr *ptr, } VIR_DEBUG("Found %Zu RADOS cluster monitors in the pool configuration", - pool->def->source.nhost); + pool->def->source.nhost); - int i; for (i = 0; i < pool->def->source.nhost; i++) { if (pool->def->source.hosts[i].name != NULL && !pool->def->source.hosts[i].port) { @@ -154,7 +155,7 @@ static int virStorageBackendRBDOpenRADOSConn(virStorageBackendRBDStatePtr *ptr, goto cleanup; } - char *mon_buff = virBufferContentAndReset(&mon_host); + mon_buff = virBufferContentAndReset(&mon_host); VIR_DEBUG("RADOS mon_host has been set to: %s", mon_buff); if (rados_conf_set(ptr->cluster, "mon_host", mon_buff) < 0) { virStorageReportError(VIR_ERR_INTERNAL_ERROR, @@ -178,6 +179,7 @@ cleanup: VIR_FREE(rados_key); virSecretFree(secret); virBufferFreeAndReset(&mon_host); + VIR_FREE(mon_buff); return ret; } Hmm, I spoke too soon - I committed the above things, then found a few more on a final review. I'm also squashing in the following. %Zu is an obsolete glibc extension; we prefer the C99 %zu instead. And you failed to fit 80 columns in a number of places. diff --git i/src/conf/storage_conf.c w/src/conf/storage_conf.c index 06334c3..bf4567f 100644 --- i/src/conf/storage_conf.c +++ w/src/conf/storage_conf.c @@ -429,8 +429,8 @@ virStoragePoolDefParseAuthCephx(xmlXPathContextPtr ctxt, uuid = virXPathString("string(./auth/secret/@uuid)", ctxt); auth->secret.usage = virXPathString("string(./auth/secret/@usage)", ctxt); if (uuid == NULL && auth->secret.usage == NULL) { - virStorageReportError(VIR_ERR_XML_ERROR, - "%s", _("missing auth secret uuid or usage attribute")); + virStorageReportError(VIR_ERR_XML_ERROR, "%s", + _("missing auth secret uuid or usage attribute")); return -1; } @@ -465,10 +465,9 @@ virStoragePoolDefParseSource(xmlXPathContextPtr ctxt, source->name = virXPathString("string(./name)", ctxt); if (pool_type == VIR_STORAGE_POOL_RBD && source->name == NULL) { - virStorageReportError(VIR_ERR_XML_ERROR, - "%s", _("missing mandatory 'name' field for RBD pool name")); - VIR_FREE(source->name); - goto cleanup; + virStorageReportError(VIR_ERR_XML_ERROR, "%s", + _("missing mandatory 'name' field for RBD pool name")); + goto cleanup; } if (options->formatFromString) { @@ -799,7 +798,8 @@ virStoragePoolDefParseXML(xmlXPathContextPtr ctxt) { } } - /* When we are working with a virtual disk we can skip the target path and permissions */ + /* When we are working with a virtual disk we can skip the target + * path and permissions */ if (!(options->flags & VIR_STORAGE_POOL_SOURCE_NETWORK)) { if ((tmppath = virXPathString("string(./target/path)", ctxt)) == NULL) { virStorageReportError(VIR_ERR_XML_ERROR, @@ -1011,7 +1011,8 @@ virStoragePoolDefFormat(virStoragePoolDefPtr def) { if (virStoragePoolSourceFormat(&buf, options, &def->source) < 0) goto cleanup; - /* RBD devices are no local block devs nor files, so it doesn't have a target */ + /* RBD devices are not local block devs nor files, so it doesn't + * have a target */ if (def->type != VIR_STORAGE_POOL_RBD) { virBufferAddLit(&buf," <target>\n"); diff --git i/src/storage/storage_backend_rbd.c w/src/storage/storage_backend_rbd.c index 46bd892..3e7464c 100644 --- i/src/storage/storage_backend_rbd.c +++ w/src/storage/storage_backend_rbd.c @@ -131,7 +131,7 @@ static int virStorageBackendRBDOpenRADOSConn(virStorageBackendRBDStatePtr *ptr, } } - VIR_DEBUG("Found %Zu RADOS cluster monitors in the pool configuration", + VIR_DEBUG("Found %zu RADOS cluster monitors in the pool configuration", pool->def->source.nhost); for (i = 0; i < pool->def->source.nhost; i++) { -- Eric Blake eblake(a)redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org