On 05/21/2012 05:00 AM, Daniel P. Berrange wrote:
> On Mon, May 14, 2012 at 11:06:42AM +0200, Wido den Hollander wrote:
>> This patch adds support for a new storage backend with RBD support.
>>
>> RBD is the RADOS Block Device and is part of the Ceph distributed storage system.
>>
>> It comes in two flavours: Qemu-RBD and Kernel RBD, this storage backend only supports
>> Qemu-RBD, thus limiting the use of this storage driver to Qemu only.
>>
>> To function this backend relies on librbd and librados being present on the local system.
>>
>> The backend also supports Cephx authentication for safe authentication with the Ceph cluster.
>>
>> For storing credentials it uses the build-in secret mechanism of libvirt.
>>
>> Signed-off-by: Wido den Hollander <wido@xxxxxxxxx>
>
> ACK to this patch
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 @@
</volume></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@xxxxxxxxxx +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list