Re: [libvirt] [PATCH v4 4/4] qemu/rbd: improve rbd device specification
On 10/28/2011 03:19 PM, Josh Durgin wrote:
From: Sage Weil
This improves the support for qemu rbd devices by adding support for a few
key features (e.g., authentication) and cleaning up the way in which
rbd configuration options are passed to qemu.
And member of the disk source xml specifies how librbd should
s/And/An/
authenticate. The username attribute is the Ceph/RBD user to authenticate as.
The usage or uuid attributes specify which secret to use. Usage is an
arbitrary identifier local to libvirt.
The old RBD support relied on setting an environment variable to
communicate information to qemu/librbd. Instead, pass those options
explicitly to qemu. Update the qemu argument parsing and tests
accordingly.
Signed-off-by: Sage Weil
Signed-off-by: Josh Durgin
---
This fixes the things Daniel mentioned.
Needs a v5, to fix memory management issues.
@@ -1495,6 +1496,159 @@ qemuSafeSerialParamValue(const char *value)
return 0;
}
+static int qemuBuildRBDString(virConnectPtr conn,
+ virDomainDiskDefPtr disk,
+ virBufferPtr opt)
Nit: We've been using this style:
static int
qemuBuildRBDString(virConnectPtr conn,
virDomainDiskDefPtr disk,
virBufferPtr opt)
+if (sec) {
+char *base64;
+
+secret = (char *)conn->secretDriver->getValue(sec,&secret_size, 0,
+
VIR_SECRET_GET_VALUE_INTERNAL_CALL);
+if (secret == NULL) {
+qemuReportError(VIR_ERR_INTERNAL_ERROR,
+_("could not get the value of the secret for
username %s"),
+disk->auth.username);
+return -1;
Resource leak. You need to ensure virUnrefSecret(sec) gets called on
this path. I'd move that particular cleanup down into the cleanup:
label, and make this part goto cleanup instead of return -1.
+}
+/* qemu/librbd wants it base64 encoded */
+base64_encode_alloc(secret, secret_size,&base64);
+virBufferEscape(opt, ":", ":key=%s:auth_supported=cephx\\;none",
+base64);
Need to check for allocation failure of base64_encode_alloc; otherwise,
you blindly passed NULL base64 to virBufferEscape, which is not portable.
+VIR_FREE(base64);
+VIR_FREE(secret);
+virUnrefSecret(sec);
+} else {
+qemuReportError(VIR_ERR_INTERNAL_ERROR,
+_("rbd username '%s' specified but secret not
found"),
+disk->auth.username);
+return -1;
+}
+}
+
+if (disk->nhosts> 0) {
+virBufferStrcat(opt, ":mon_host=", NULL);
+for (i = 0; i< disk->nhosts; ++i) {
+if (i) {
+virBufferStrcat(opt, "\\;", NULL);
virBufferAddLit(opt, "\\;")
is more efficient than virBufferStrcat().
+}
+if (disk->hosts[i].port) {
+virBufferAsprintf(opt, "%s\\:%s",
+ disk->hosts[i].name,
+ disk->hosts[i].port);
+} else {
+virBufferAsprintf(opt, "%s", disk->hosts[i].name);
+}
+}
+}
+
+return 0;
+}
+
+static int qemuAddRBDHost(virDomainDiskDefPtr disk, char *hostport)
+{
+char *port;
+int ret;
+
+disk->nhosts++;
+ret = VIR_REALLOC_N(disk->hosts, disk->nhosts);
+if (ret< 0) {
+virReportOOMError();
+return -1;
+}
+
+port = strstr(hostport, "\\:");
+if (port) {
+*port = '\0';
+port += 2;
+disk->hosts[disk->nhosts-1].port = strdup(port);
+} else {
+disk->hosts[disk->nhosts-1].port = strdup("6789");
+}
+disk->hosts[disk->nhosts-1].name = strdup(hostport);
These three strdup() need to check for allocation failure.
+return 0;
+}
+
+/* disk->src initially has everything after the rbd: prefix */
+static int qemuParseRBDString(virDomainDiskDefPtr disk)
+{
+char *options = NULL;
+char *p, *e, *next;
+
+p = strchr(disk->src, ':');
+if (p) {
+options = strdup(p + 1);
+*p = '\0';
Need to check for strdup() failure.
+}
+
+/* options */
+if (!options)
+return 0; /* all done */
+
+p = options;
+while (*p) {
+/* find : delimiter or end of string */
+for (e = p; *e&& *e != ':'; ++e) {
+if (*e == '\\') {
+e++;
+if (*e == '\0')
+break;
+}
+}
+if (*e == '\0') {
+next = e;/* last kv pair */
+} else {
+next = e + 1;
+*e = '\0';
+}
+
+if (STRPREFIX(p, "id=")) {
+disk->auth.username = strdup(p + strlen("id="));
Check for allocation failure. Also, you might want to se
[libvirt] [PATCH v4 4/4] qemu/rbd: improve rbd device specification
From: Sage Weil
This improves the support for qemu rbd devices by adding support for a few
key features (e.g., authentication) and cleaning up the way in which
rbd configuration options are passed to qemu.
And member of the disk source xml specifies how librbd should
authenticate. The username attribute is the Ceph/RBD user to authenticate as.
The usage or uuid attributes specify which secret to use. Usage is an
arbitrary identifier local to libvirt.
The old RBD support relied on setting an environment variable to
communicate information to qemu/librbd. Instead, pass those options
explicitly to qemu. Update the qemu argument parsing and tests
accordingly.
Signed-off-by: Sage Weil
Signed-off-by: Josh Durgin
---
This fixes the things Daniel mentioned.
src/qemu/qemu_command.c| 284
.../qemuxml2argv-disk-drive-network-rbd-auth.args |6 +
.../qemuxml2argv-disk-drive-network-rbd-auth.xml | 37 +++
.../qemuxml2argv-disk-drive-network-rbd.args |6 +-
tests/qemuxml2argvtest.c | 56
5 files changed, 272 insertions(+), 117 deletions(-)
create mode 100644
tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-auth.args
create mode 100644
tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-auth.xml
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index f5d89b9..48b0762 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -38,6 +38,7 @@
#include "domain_audit.h"
#include "domain_conf.h"
#include "network/bridge_driver.h"
+#include "base64.h"
#include
#include
@@ -1495,6 +1496,159 @@ qemuSafeSerialParamValue(const char *value)
return 0;
}
+static int qemuBuildRBDString(virConnectPtr conn,
+ virDomainDiskDefPtr disk,
+ virBufferPtr opt)
+{
+int i;
+virSecretPtr sec = NULL;
+char *secret = NULL;
+size_t secret_size;
+
+virBufferAsprintf(opt, "rbd:%s", disk->src);
+if (disk->auth.username) {
+virBufferEscape(opt, ":", ":id=%s", disk->auth.username);
+/* 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,
+ VIR_SECRET_USAGE_TYPE_CEPH,
+ disk->auth.secret.usage);
+break;
+}
+
+if (sec) {
+char *base64;
+
+secret = (char *)conn->secretDriver->getValue(sec, &secret_size, 0,
+
VIR_SECRET_GET_VALUE_INTERNAL_CALL);
+if (secret == NULL) {
+qemuReportError(VIR_ERR_INTERNAL_ERROR,
+_("could not get the value of the secret for
username %s"),
+disk->auth.username);
+return -1;
+}
+/* qemu/librbd wants it base64 encoded */
+base64_encode_alloc(secret, secret_size, &base64);
+virBufferEscape(opt, ":", ":key=%s:auth_supported=cephx\\;none",
+base64);
+VIR_FREE(base64);
+VIR_FREE(secret);
+virUnrefSecret(sec);
+} else {
+qemuReportError(VIR_ERR_INTERNAL_ERROR,
+_("rbd username '%s' specified but secret not
found"),
+disk->auth.username);
+return -1;
+}
+}
+
+if (disk->nhosts > 0) {
+virBufferStrcat(opt, ":mon_host=", NULL);
+for (i = 0; i < disk->nhosts; ++i) {
+if (i) {
+virBufferStrcat(opt, "\\;", NULL);
+}
+if (disk->hosts[i].port) {
+virBufferAsprintf(opt, "%s\\:%s",
+ disk->hosts[i].name,
+ disk->hosts[i].port);
+} else {
+virBufferAsprintf(opt, "%s", disk->hosts[i].name);
+}
+}
+}
+
+return 0;
+}
+
+static int qemuAddRBDHost(virDomainDiskDefPtr disk, char *hostport)
+{
+char *port;
+int ret;
+
+disk->nhosts++;
+ret = VIR_REALLOC_N(disk->hosts, disk->nhosts);
+if (ret < 0) {
+virReportOOMError();
+return -1;
+}
+
+port = strstr(hostport, "\\:");
+if (port) {
+*port = '\0';
+port += 2;
+disk->hosts[disk->nhosts-1].port = strdup(port);
+} else {
+disk->hosts[disk->nhosts-1].port = strdup("6789");
+}
+disk->hosts[disk->nhosts-1].name = strdup(hostport);
+return 0;
+}
+
+/* disk->src initially has everything after the rbd: prefix */
+static int qemuParseRBDString(virDomainDiskDefPtr d
