Re: [libvirt] [RFC PATCH v3 4/4] qemu/rbd: improve rbd device specification
On Thu, Oct 20, 2011 at 11:01:27AM -0700, Josh Durgin wrote:
From: Sage Weil <sage@newdream.net>
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 <auth> 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 <sage@newdream.net> Signed-off-by: Josh Durgin <josh.durgin@dreamhost.com> --- 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 | 52 ++++ 5 files changed, 268 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 4dfce88..b2c0eee 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 <sys/utsname.h> #include <sys/stat.h> @@ -1489,6 +1490,159 @@ qemuSafeSerialParamValue(const char *value) return 0; }
+static int buildRBDString(virConnectPtr conn, + virDomainDiskDefPtr disk, + virBufferPtr opt)
For style reasons, s/buildRBDString/qemuBuildRBDString/
+{ + 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 addRBDHost(virDomainDiskDefPtr disk, char *hostport)
s/add/qemuAdd/
+{ + 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 parseRBDString(virDomainDiskDefPtr disk)
s/parse/qemuParse/
+{ + char *options = NULL; + char *p, *e, *next; + + p = strchr(disk->src, ':'); + if (p) { + options = strdup(p + 1); + *p = '\0'; + } + + /* 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=")); + } + if (STRPREFIX(p, "mon_host=")) { + char *h, *sep; + + h = p + strlen("mon_host="); + while (h < e) { + for (sep = h; sep < e; ++sep) { + if (*sep == '\\' && (sep[1] == ',' || + sep[1] == ';' || + sep[1] == ' ')) { + *sep = '\0'; + sep += 2; + break; + } + } + addRBDHost(disk, h); + h = sep; + } + } + + p = next; + } + return 0; +}
char * qemuBuildDriveStr(virConnectPtr conn, @@ -1608,8 +1762,10 @@ qemuBuildDriveStr(virConnectPtr conn, disk->hosts->name, disk->hosts->port); break; case VIR_DOMAIN_DISK_PROTOCOL_RBD: - /* TODO: set monitor hostnames */ - virBufferAsprintf(&opt, "file=rbd:%s,", disk->src); + virBufferStrcat(&opt, "file=", NULL); + if (buildRBDString(conn, disk, &opt) < 0) + goto error; + virBufferStrcat(&opt, ",", NULL); break; case VIR_DOMAIN_DISK_PROTOCOL_SHEEPDOG: if (disk->nhosts == 0) @@ -3278,8 +3434,6 @@ qemuBuildCommandLine(virConnectPtr conn, int last_good_net = -1; bool hasHwVirt = false; virCommandPtr cmd; - bool has_rbd_hosts = false; - virBuffer rbd_hosts = VIR_BUFFER_INITIALIZER; bool emitBootindex = false; int usbcontroller = 0; bool usblegacy = false; @@ -3861,7 +4015,6 @@ qemuBuildCommandLine(virConnectPtr conn, virDomainDiskDefPtr disk = def->disks[i]; int withDeviceArg = 0; bool deviceFlagMasked = false; - int j;
/* Unless we have -device, then USB disks need special handling */ @@ -3919,26 +4072,6 @@ qemuBuildCommandLine(virConnectPtr conn, virCommandAddArg(cmd, optstr); VIR_FREE(optstr);
- if (disk->type == VIR_DOMAIN_DISK_TYPE_NETWORK && - disk->protocol == VIR_DOMAIN_DISK_PROTOCOL_RBD) { - for (j = 0 ; j < disk->nhosts ; j++) { - if (!has_rbd_hosts) { - virBufferAddLit(&rbd_hosts, "CEPH_ARGS=-m "); - has_rbd_hosts = true; - } else { - virBufferAddLit(&rbd_hosts, ","); - } - virDomainDiskHostDefPtr host = &disk->hosts[j]; - if (host->port) { - virBufferAsprintf(&rbd_hosts, "%s:%s", - host->name, - host->port); - } else { - virBufferAdd(&rbd_hosts, host->name, -1); - } - } - } - if (!emitBootindex) bootindex = 0; else if (disk->bootIndex) @@ -3976,7 +4109,6 @@ qemuBuildCommandLine(virConnectPtr conn, char *file; const char *fmt; virDomainDiskDefPtr disk = def->disks[i]; - int j;
if (disk->bus == VIR_DOMAIN_DISK_BUS_USB) { if (disk->device == VIR_DOMAIN_DISK_DEVICE_DISK) { @@ -4045,24 +4177,15 @@ qemuBuildCommandLine(virConnectPtr conn, } break; case VIR_DOMAIN_DISK_PROTOCOL_RBD: - if (virAsprintf(&file, "rbd:%s,", disk->src) < 0) { - goto no_memory; - } - for (j = 0 ; j < disk->nhosts ; j++) { - if (!has_rbd_hosts) { - virBufferAddLit(&rbd_hosts, "CEPH_ARGS=-m "); - has_rbd_hosts = true; - } else { - virBufferAddLit(&rbd_hosts, ","); - } - virDomainDiskHostDefPtr host = &disk->hosts[j]; - if (host->port) { - virBufferAsprintf(&rbd_hosts, "%s:%s", - host->name, - host->port); - } else { - virBufferAdd(&rbd_hosts, host->name, -1); + { + virBuffer opt = VIR_BUFFER_INITIALIZER; + if (buildRBDString(conn, disk, &opt) < 0) + goto error; + if (virBufferError(&opt)) { + virReportOOMError(); + goto error; } + file = virBufferContentAndReset(&opt); } break; case VIR_DOMAIN_DISK_PROTOCOL_SHEEPDOG: @@ -4091,9 +4214,6 @@ qemuBuildCommandLine(virConnectPtr conn, } }
- if (has_rbd_hosts) - virCommandAddEnvBuffer(cmd, &rbd_hosts); - if (qemuCapsGet(qemuCaps, QEMU_CAPS_FSDEV)) { for (i = 0 ; i < def->nfss ; i++) { char *optstr; @@ -5263,7 +5383,6 @@ qemuBuildCommandLine(virConnectPtr conn, networkReleaseActualDevice(def->nets[i]); for (i = 0; i <= last_good_net; i++) virDomainConfNWFilterTeardown(def->nets[i]); - virBufferFreeAndReset(&rbd_hosts); virCommandFree(cmd); return NULL; } @@ -5295,10 +5414,6 @@ static int qemuStringToArgvEnv(const char *args, const char *next;
start = curr; - /* accept a space in CEPH_ARGS */ - if (STRPREFIX(curr, "CEPH_ARGS=-m ")) { - start += strlen("CEPH_ARGS=-m "); - } if (*start == '\'') { if (start == curr) curr++; @@ -5577,6 +5692,8 @@ qemuParseCommandLineDisk(virCapsPtr caps, virReportOOMError(); goto cleanup; } + if (parseRBDString(def) < 0) + goto cleanup;
VIR_FREE(p); } else if (STRPREFIX(def->src, "sheepdog:")) { @@ -6696,7 +6813,8 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps, disk->src = NULL; break; case VIR_DOMAIN_DISK_PROTOCOL_RBD: - /* handled later since the hosts for all disks are in CEPH_ARGS */ + if (parseRBDString(disk) < 0) + goto error; break; case VIR_DOMAIN_DISK_PROTOCOL_SHEEPDOG: /* disk->src must be [vdiname] or [host]:[port]:[vdiname] */ @@ -7035,68 +7153,6 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps, }
#undef WANT_VALUE - if (def->ndisks > 0) { - const char *ceph_args = qemuFindEnv(progenv, "CEPH_ARGS"); - if (ceph_args) { - char *hosts, *port, *saveptr = NULL, *token; - virDomainDiskDefPtr first_rbd_disk = NULL; - for (i = 0 ; i < def->ndisks ; i++) { - if (def->disks[i]->type == VIR_DOMAIN_DISK_TYPE_NETWORK && - def->disks[i]->protocol == VIR_DOMAIN_DISK_PROTOCOL_RBD) { - first_rbd_disk = def->disks[i]; - break; - } - } - - if (!first_rbd_disk) { - qemuReportError(VIR_ERR_INTERNAL_ERROR, - _("CEPH_ARGS was set without an rbd disk")); - goto error; - } - - /* CEPH_ARGS should be: -m host1[:port1][,host2[:port2]]... */ - if (!STRPREFIX(ceph_args, "-m ")) { - qemuReportError(VIR_ERR_INTERNAL_ERROR, - _("could not parse CEPH_ARGS '%s'"), ceph_args); - goto error; - } - hosts = strdup(strchr(ceph_args, ' ') + 1); - if (!hosts) - goto no_memory; - first_rbd_disk->nhosts = 0; - token = strtok_r(hosts, ",", &saveptr); - while (token != NULL) { - if (VIR_REALLOC_N(first_rbd_disk->hosts, first_rbd_disk->nhosts + 1) < 0) { - VIR_FREE(hosts); - goto no_memory; - } - port = strchr(token, ':'); - if (port) { - *port++ = '\0'; - port = strdup(port); - if (!port) { - VIR_FREE(hosts); - goto no_memory; - } - } - first_rbd_disk->hosts[first_rbd_disk->nhosts].port = port; - first_rbd_disk->hosts[first_rbd_disk->nhosts].name = strdup(token); - if (!first_rbd_disk->hosts[first_rbd_disk->nhosts].name) { - VIR_FREE(hosts); - goto no_memory; - } - first_rbd_disk->nhosts++; - token = strtok_r(NULL, ",", &saveptr); - } - VIR_FREE(hosts); - - if (first_rbd_disk->nhosts == 0) { - qemuReportError(VIR_ERR_INTERNAL_ERROR, - _("found no rbd hosts in CEPH_ARGS '%s'"), ceph_args); - goto error; - } - } - }
if (!nographics && def->ngraphics == 0) { virDomainGraphicsDefPtr sdl;
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index a7ae925..31bd928 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -23,6 +23,55 @@ static const char *abs_top_srcdir; static struct qemud_driver driver;
+static unsigned char * +fakeSecretGetValue(virSecretPtr obj ATTRIBUTE_UNUSED, + size_t *value_size, + unsigned int fakeflags ATTRIBUTE_UNUSED, + unsigned int internalFlags ATTRIBUTE_UNUSED) +{ + char *secret = strdup("AQCVn5hO6HzFAhAAq0NCv8jtJcIcE+HOBlMQ1A"); + *value_size = strlen(secret); + return (unsigned char *) secret; +} + +static virSecretPtr +fakeSecretLookupByUsage(virConnectPtr conn, + int usageType ATTRIBUTE_UNUSED, + const char *usageID) +{ + virSecretPtr ret = NULL; + if (strcmp(usageID, "mycluster_myname"))
s/strcmp/STRNEQ/
+ return ret; + ret = malloc(sizeof(virSecret));
Need to use VIR_ALLOC for this.
+ ret->magic = VIR_SECRET_MAGIC; + ret->conn = conn; + conn->refs++; + ret->refs = 1; + ret->usageID = strdup(usageID); + return ret; +} +
Regards, 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 :|
From: Sage Weil <sage@newdream.net> 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 <auth> 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 <sage@newdream.net> Signed-off-by: Josh Durgin <josh.durgin@dreamhost.com> --- 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 <sys/utsname.h> #include <sys/stat.h> @@ -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 disk) +{ + char *options = NULL; + char *p, *e, *next; + + p = strchr(disk->src, ':'); + if (p) { + options = strdup(p + 1); + *p = '\0'; + } + + /* 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=")); + } + if (STRPREFIX(p, "mon_host=")) { + char *h, *sep; + + h = p + strlen("mon_host="); + while (h < e) { + for (sep = h; sep < e; ++sep) { + if (*sep == '\\' && (sep[1] == ',' || + sep[1] == ';' || + sep[1] == ' ')) { + *sep = '\0'; + sep += 2; + break; + } + } + qemuAddRBDHost(disk, h); + h = sep; + } + } + + p = next; + } + return 0; +} char * qemuBuildDriveStr(virConnectPtr conn, @@ -1614,8 +1768,10 @@ qemuBuildDriveStr(virConnectPtr conn, disk->hosts->name, disk->hosts->port); break; case VIR_DOMAIN_DISK_PROTOCOL_RBD: - /* TODO: set monitor hostnames */ - virBufferAsprintf(&opt, "file=rbd:%s,", disk->src); + virBufferStrcat(&opt, "file=", NULL); + if (qemuBuildRBDString(conn, disk, &opt) < 0) + goto error; + virBufferStrcat(&opt, ",", NULL); break; case VIR_DOMAIN_DISK_PROTOCOL_SHEEPDOG: if (disk->nhosts == 0) @@ -3284,8 +3440,6 @@ qemuBuildCommandLine(virConnectPtr conn, int last_good_net = -1; bool hasHwVirt = false; virCommandPtr cmd; - bool has_rbd_hosts = false; - virBuffer rbd_hosts = VIR_BUFFER_INITIALIZER; bool emitBootindex = false; int usbcontroller = 0; bool usblegacy = false; @@ -3865,7 +4019,6 @@ qemuBuildCommandLine(virConnectPtr conn, virDomainDiskDefPtr disk = def->disks[i]; int withDeviceArg = 0; bool deviceFlagMasked = false; - int j; /* Unless we have -device, then USB disks need special handling */ @@ -3923,26 +4076,6 @@ qemuBuildCommandLine(virConnectPtr conn, virCommandAddArg(cmd, optstr); VIR_FREE(optstr); - if (disk->type == VIR_DOMAIN_DISK_TYPE_NETWORK && - disk->protocol == VIR_DOMAIN_DISK_PROTOCOL_RBD) { - for (j = 0 ; j < disk->nhosts ; j++) { - if (!has_rbd_hosts) { - virBufferAddLit(&rbd_hosts, "CEPH_ARGS=-m "); - has_rbd_hosts = true; - } else { - virBufferAddLit(&rbd_hosts, ","); - } - virDomainDiskHostDefPtr host = &disk->hosts[j]; - if (host->port) { - virBufferAsprintf(&rbd_hosts, "%s:%s", - host->name, - host->port); - } else { - virBufferAdd(&rbd_hosts, host->name, -1); - } - } - } - if (!emitBootindex) bootindex = 0; else if (disk->bootIndex) @@ -3980,7 +4113,6 @@ qemuBuildCommandLine(virConnectPtr conn, char *file; const char *fmt; virDomainDiskDefPtr disk = def->disks[i]; - int j; if (disk->bus == VIR_DOMAIN_DISK_BUS_USB) { if (disk->device == VIR_DOMAIN_DISK_DEVICE_DISK) { @@ -4049,24 +4181,15 @@ qemuBuildCommandLine(virConnectPtr conn, } break; case VIR_DOMAIN_DISK_PROTOCOL_RBD: - if (virAsprintf(&file, "rbd:%s,", disk->src) < 0) { - goto no_memory; - } - for (j = 0 ; j < disk->nhosts ; j++) { - if (!has_rbd_hosts) { - virBufferAddLit(&rbd_hosts, "CEPH_ARGS=-m "); - has_rbd_hosts = true; - } else { - virBufferAddLit(&rbd_hosts, ","); - } - virDomainDiskHostDefPtr host = &disk->hosts[j]; - if (host->port) { - virBufferAsprintf(&rbd_hosts, "%s:%s", - host->name, - host->port); - } else { - virBufferAdd(&rbd_hosts, host->name, -1); + { + 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_SHEEPDOG: @@ -4095,9 +4218,6 @@ qemuBuildCommandLine(virConnectPtr conn, } } - if (has_rbd_hosts) - virCommandAddEnvBuffer(cmd, &rbd_hosts); - if (qemuCapsGet(qemuCaps, QEMU_CAPS_FSDEV)) { for (i = 0 ; i < def->nfss ; i++) { char *optstr; @@ -5267,7 +5387,6 @@ qemuBuildCommandLine(virConnectPtr conn, networkReleaseActualDevice(def->nets[i]); for (i = 0; i <= last_good_net; i++) virDomainConfNWFilterTeardown(def->nets[i]); - virBufferFreeAndReset(&rbd_hosts); virCommandFree(cmd); return NULL; } @@ -5299,10 +5418,6 @@ static int qemuStringToArgvEnv(const char *args, const char *next; start = curr; - /* accept a space in CEPH_ARGS */ - if (STRPREFIX(curr, "CEPH_ARGS=-m ")) { - start += strlen("CEPH_ARGS=-m "); - } if (*start == '\'') { if (start == curr) curr++; @@ -5581,6 +5696,8 @@ qemuParseCommandLineDisk(virCapsPtr caps, virReportOOMError(); goto cleanup; } + if (qemuParseRBDString(def) < 0) + goto cleanup; VIR_FREE(p); } else if (STRPREFIX(def->src, "sheepdog:")) { @@ -6700,7 +6817,8 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps, disk->src = NULL; break; case VIR_DOMAIN_DISK_PROTOCOL_RBD: - /* handled later since the hosts for all disks are in CEPH_ARGS */ + if (qemuParseRBDString(disk) < 0) + goto error; break; case VIR_DOMAIN_DISK_PROTOCOL_SHEEPDOG: /* disk->src must be [vdiname] or [host]:[port]:[vdiname] */ @@ -7039,68 +7157,6 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps, } #undef WANT_VALUE - if (def->ndisks > 0) { - const char *ceph_args = qemuFindEnv(progenv, "CEPH_ARGS"); - if (ceph_args) { - char *hosts, *port, *saveptr = NULL, *token; - virDomainDiskDefPtr first_rbd_disk = NULL; - for (i = 0 ; i < def->ndisks ; i++) { - if (def->disks[i]->type == VIR_DOMAIN_DISK_TYPE_NETWORK && - def->disks[i]->protocol == VIR_DOMAIN_DISK_PROTOCOL_RBD) { - first_rbd_disk = def->disks[i]; - break; - } - } - - if (!first_rbd_disk) { - qemuReportError(VIR_ERR_INTERNAL_ERROR, - _("CEPH_ARGS was set without an rbd disk")); - goto error; - } - - /* CEPH_ARGS should be: -m host1[:port1][,host2[:port2]]... */ - if (!STRPREFIX(ceph_args, "-m ")) { - qemuReportError(VIR_ERR_INTERNAL_ERROR, - _("could not parse CEPH_ARGS '%s'"), ceph_args); - goto error; - } - hosts = strdup(strchr(ceph_args, ' ') + 1); - if (!hosts) - goto no_memory; - first_rbd_disk->nhosts = 0; - token = strtok_r(hosts, ",", &saveptr); - while (token != NULL) { - if (VIR_REALLOC_N(first_rbd_disk->hosts, first_rbd_disk->nhosts + 1) < 0) { - VIR_FREE(hosts); - goto no_memory; - } - port = strchr(token, ':'); - if (port) { - *port++ = '\0'; - port = strdup(port); - if (!port) { - VIR_FREE(hosts); - goto no_memory; - } - } - first_rbd_disk->hosts[first_rbd_disk->nhosts].port = port; - first_rbd_disk->hosts[first_rbd_disk->nhosts].name = strdup(token); - if (!first_rbd_disk->hosts[first_rbd_disk->nhosts].name) { - VIR_FREE(hosts); - goto no_memory; - } - first_rbd_disk->nhosts++; - token = strtok_r(NULL, ",", &saveptr); - } - VIR_FREE(hosts); - - if (first_rbd_disk->nhosts == 0) { - qemuReportError(VIR_ERR_INTERNAL_ERROR, - _("found no rbd hosts in CEPH_ARGS '%s'"), ceph_args); - goto error; - } - } - } if (!nographics && def->ngraphics == 0) { virDomainGraphicsDefPtr sdl; diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-auth.args b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-auth.args new file mode 100644 index 0000000..f827615 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-auth.args @@ -0,0 +1,6 @@ +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 -drive \ +file=/dev/HostVG/QEMUGuest1,if=ide,bus=0,unit=0 -drive \ +file=rbd:pool/image:id=myname:key=QVFDVm41aE82SHpGQWhBQXEwTkN2OGp0SmNJY0UrSE9CbE1RMUE=:auth_supported=cephx\;none:mon_host=mon1.example.org\:6321\;mon2.example.org\:6322\;mon3.example.org\:6322,\ +if=virtio,format=raw -net none -serial none -parallel none -usb diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-auth.xml b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-auth.xml new file mode 100644 index 0000000..88f7f7a --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-auth.xml @@ -0,0 +1,37 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory>219136</memory> + <currentMemory>219136</currentMemory> + <vcpu>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' unit='0'/> + </disk> + <disk type='network' device='disk'> + <driver name='qemu' type='raw'/> + <auth username='myname'> + <secret type='ceph' usage='mycluster_myname'/> + </auth> + <source protocol='rbd' name='pool/image'> + <host name='mon1.example.org' port='6321'/> + <host name='mon2.example.org' port='6322'/> + <host name='mon3.example.org' port='6322'/> + </source> + <target dev='vda' bus='virtio'/> + </disk> + <controller type='ide' index='0'/> + <memballoon model='virtio'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd.args b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd.args index 3ab1219..50651a5 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd.args @@ -1,6 +1,6 @@ -LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test CEPH_ARGS=-m \ -mon1.example.org:6321,mon2.example.org:6322,mon3.example.org:6322 \ +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 -drive \ -file=/dev/HostVG/QEMUGuest1,if=ide,bus=0,unit=0 -drive file=rbd:pool/image,\ +file=/dev/HostVG/QEMUGuest1,if=ide,bus=0,unit=0 -drive \ +file=rbd:pool/image:mon_host=mon1.example.org\:6321\;mon2.example.org\:6322\;mon3.example.org\:6322,\ if=virtio,format=raw -net none -serial none -parallel none -usb diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 4d6db01..34017aa 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -12,6 +12,7 @@ # include "internal.h" # include "testutils.h" +# include "util/memory.h" # include "qemu/qemu_capabilities.h" # include "qemu/qemu_command.h" # include "qemu/qemu_domain.h" @@ -23,6 +24,58 @@ static const char *abs_top_srcdir; static struct qemud_driver driver; +static unsigned char * +fakeSecretGetValue(virSecretPtr obj ATTRIBUTE_UNUSED, + size_t *value_size, + unsigned int fakeflags ATTRIBUTE_UNUSED, + unsigned int internalFlags ATTRIBUTE_UNUSED) +{ + char *secret = strdup("AQCVn5hO6HzFAhAAq0NCv8jtJcIcE+HOBlMQ1A"); + *value_size = strlen(secret); + return (unsigned char *) secret; +} + +static virSecretPtr +fakeSecretLookupByUsage(virConnectPtr conn, + int usageType ATTRIBUTE_UNUSED, + const char *usageID) +{ + virSecretPtr ret = NULL; + int err; + if (STRNEQ(usageID, "mycluster_myname")) + return NULL; + err = VIR_ALLOC(ret); + if (err < 0) + return NULL; + ret->magic = VIR_SECRET_MAGIC; + ret->conn = conn; + conn->refs++; + ret->refs = 1; + ret->usageID = strdup(usageID); + return ret; +} + +static int +fakeSecretClose(virConnectPtr conn ATTRIBUTE_UNUSED) +{ + return 0; +} + +static virSecretDriver fakeSecretDriver = { + .name = "fake_secret", + .open = NULL, + .close = fakeSecretClose, + .numOfSecrets = NULL, + .listSecrets = NULL, + .lookupByUUID = NULL, + .lookupByUsage = fakeSecretLookupByUsage, + .defineXML = NULL, + .getXMLDesc = NULL, + .setValue = NULL, + .getValue = fakeSecretGetValue, + .undefine = NULL, +}; + static int testCompareXMLToArgvFiles(const char *xml, const char *cmdline, virBitmapPtr extraFlags, @@ -44,6 +97,7 @@ static int testCompareXMLToArgvFiles(const char *xml, if (!(conn = virGetConnect())) goto fail; + conn->secretDriver = &fakeSecretDriver; len = virtTestLoadFile(cmdline, &expectargv); if (len < 0) @@ -357,6 +411,8 @@ mymain(void) QEMU_CAPS_DRIVE, QEMU_CAPS_DRIVE_FORMAT); DO_TEST("disk-drive-network-sheepdog", false, QEMU_CAPS_DRIVE, QEMU_CAPS_DRIVE_FORMAT); + DO_TEST("disk-drive-network-rbd-auth", false, + QEMU_CAPS_DRIVE, QEMU_CAPS_DRIVE_FORMAT); DO_TEST("disk-drive-no-boot", false, QEMU_CAPS_DRIVE, QEMU_CAPS_DEVICE, QEMU_CAPS_BOOTINDEX); DO_TEST("disk-usb", false, NONE); -- 1.7.1
The types used in domaincommon.rng and secret.rng should be the same. Signed-off-by: Josh Durgin <josh.durgin@dreamhost.com> --- docs/schemas/domaincommon.rng | 11 ++++++++--- docs/schemas/secret.rng | 4 +++- 2 files changed, 11 insertions(+), 4 deletions(-) diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 3477351..d053489 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -2558,13 +2558,13 @@ <attribute name='uuid'> <ref name="UUID"/> </attribute> - <attribute name="usage"> - <ref name="genericName"/> + <attribute name='usage'> + <ref name='usageName'/> </attribute> </choice> </element> </define> - + <!-- Optional hypervisor extensions in their own namespace: QEmu @@ -2675,6 +2675,11 @@ <param name="pattern">(([0-2]?[0-9]?[0-9]\.){3}[0-2]?[0-9]?[0-9])|(([0-9a-fA-F]+|:)+[0-9a-fA-F]+)|([a-zA-Z0-9_\.\+\-]*)</param> </data> </define> + <define name="usageName"> + <data type="string"> + <param name="pattern">[a-zA-Z0-9_\.\+\-]+</param> + </data> + </define> <define name="usbId"> <data type="string"> <param name="pattern">(0x)?[0-9a-fA-F]{1,4}</param> diff --git a/docs/schemas/secret.rng b/docs/schemas/secret.rng index 8e7714b..3abd3c7 100644 --- a/docs/schemas/secret.rng +++ b/docs/schemas/secret.rng @@ -4,6 +4,8 @@ <ref name='secret'/> </start> + <include href='domaincommon.rng'/> + <define name='secret'> <element name='secret'> <optional> @@ -60,7 +62,7 @@ <value>ceph</value> </attribute> <element name='name'> - <text/> + <ref name='usageName'/> </element> </define> -- 1.7.1
On 10/28/2011 03:19 PM, Josh Durgin wrote:
The types used in domaincommon.rng and secret.rng should be the same.
Signed-off-by: Josh Durgin<josh.durgin@dreamhost.com> --- docs/schemas/domaincommon.rng | 11 ++++++++--- docs/schemas/secret.rng | 4 +++- 2 files changed, 11 insertions(+), 4 deletions(-)
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 3477351..d053489 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -2558,13 +2558,13 @@ <attribute name='uuid'> <ref name="UUID"/> </attribute> -<attribute name="usage"> -<ref name="genericName"/> +<attribute name='usage'> +<ref name='usageName'/>
genericName is probably fine instead of inventing usageName; but I'll graduate it out of domaincommon.rng into basictypes.rng.
</attribute> </choice> </element> </define> - +
Spurious whitespace change.
+++ b/docs/schemas/secret.rng @@ -4,6 +4,8 @@ <ref name='secret'/> </start>
+<include href='domaincommon.rng'/>
domaincommon.rng is rather heavyweight; basictypes.rng is better.
+ <define name='secret'> <element name='secret'> <optional> @@ -60,7 +62,7 @@ <value>ceph</value> </attribute> <element name='name'> -<text/> +<ref name='usageName'/> </element> </define>
At any rate, once we include common types, we can stop repeating the UUID definition too. Not quite like your original patch, but I'll go ahead and squash this in, then push in your name since it matches up with your intent. diff --git i/docs/schemas/basictypes.rng w/docs/schemas/basictypes.rng index b3267f5..3b4b952 100644 --- i/docs/schemas/basictypes.rng +++ w/docs/schemas/basictypes.rng @@ -97,6 +97,12 @@ </choice> </define> + <define name="genericName"> + <data type="string"> + <param name="pattern">[a-zA-Z0-9_\+\-]+</param> + </data> + </define> + <define name="dnsName"> <data type="string"> <param name="pattern">[a-zA-Z0-9\.\-]+</param> diff --git i/docs/schemas/domaincommon.rng w/docs/schemas/domaincommon.rng index d053489..b6f858e 100644 --- i/docs/schemas/domaincommon.rng +++ w/docs/schemas/domaincommon.rng @@ -2559,12 +2559,12 @@ <ref name="UUID"/> </attribute> <attribute name='usage'> - <ref name='usageName'/> + <ref name='genericName'/> </attribute> </choice> </element> </define> - + <!-- Optional hypervisor extensions in their own namespace: QEmu @@ -2660,11 +2660,6 @@ <param name="pattern">[A-Za-z0-9_\.\+\-]+</param> </data> </define> - <define name="genericName"> - <data type="string"> - <param name="pattern">[a-zA-Z0-9_\+\-]+</param> - </data> - </define> <define name="bridgeMode"> <data type="string"> <param name="pattern">(vepa|bridge|private|passthrough)</param> @@ -2675,11 +2670,6 @@ <param name="pattern">(([0-2]?[0-9]?[0-9]\.){3}[0-2]?[0-9]?[0-9])|(([0-9a-fA-F]+|:)+[0-9a-fA-F]+)|([a-zA-Z0-9_\.\+\-]*)</param> </data> </define> - <define name="usageName"> - <data type="string"> - <param name="pattern">[a-zA-Z0-9_\.\+\-]+</param> - </data> - </define> <define name="usbId"> <data type="string"> <param name="pattern">(0x)?[0-9a-fA-F]{1,4}</param> diff --git i/docs/schemas/secret.rng w/docs/schemas/secret.rng index 3abd3c7..e49bd5a 100644 --- i/docs/schemas/secret.rng +++ w/docs/schemas/secret.rng @@ -1,10 +1,11 @@ +<?xml version="1.0"?> <!-- A Relax NG schema for the libvirt secret properties XML format --> <grammar xmlns="http://relaxng.org/ns/structure/1.0"> <start> <ref name='secret'/> </start> - <include href='domaincommon.rng'/> + <include href='basictypes.rng'/> <define name='secret'> <element name='secret'> @@ -62,25 +63,8 @@ <value>ceph</value> </attribute> <element name='name'> - <ref name='usageName'/> + <ref name='genericName'/> </element> </define> - <define name="UUID"> - <choice> - <data type="string"> - <param name="pattern">[a-fA-F0-9]{32}</param> - </data> - <data type="string"> - <param name="pattern">[a-fA-F0-9]{8}\-([a-fA-F0-9]{4}\-){3}[a-fA-F0-9]{12}</param> - </data> - </choice> - </define> - - <define name="absFilePath"> - <data type="string"> - <param name="pattern">/[a-zA-Z0-9_\.\+\-&/%]+</param> - </data> - </define> - </grammar> -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On 10/28/2011 03:19 PM, Josh Durgin wrote:
From: Sage Weil<sage@newdream.net>
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<auth> 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<sage@newdream.net> Signed-off-by: Josh Durgin<josh.durgin@dreamhost.com> ---
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 see if using strndup() on the original string is easier than copying the string, just so you can do in-place modifications of injecting NUL bytes for strdup() to work.
+ } + if (STRPREFIX(p, "mon_host=")) { + char *h, *sep; + + h = p + strlen("mon_host="); + while (h< e) { + for (sep = h; sep< e; ++sep) { + if (*sep == '\\'&& (sep[1] == ',' || + sep[1] == ';' || + sep[1] == ' ')) { + *sep = '\0'; + sep += 2; + break; + } + } + qemuAddRBDHost(disk, h);
Don't ignore the return value here.
+ h = sep; + } + } + + p = next; + } + return 0; +}
char * qemuBuildDriveStr(virConnectPtr conn, @@ -1614,8 +1768,10 @@ qemuBuildDriveStr(virConnectPtr conn, disk->hosts->name, disk->hosts->port); break; case VIR_DOMAIN_DISK_PROTOCOL_RBD: - /* TODO: set monitor hostnames */ - virBufferAsprintf(&opt, "file=rbd:%s,", disk->src); + virBufferStrcat(&opt, "file=", NULL);
virBufferAddLit(&opt, "file=") is more efficient.
+ if (qemuBuildRBDString(conn, disk,&opt)< 0) + goto error; + virBufferStrcat(&opt, ",", NULL);
virBufferAddChar(&opt, ',') is more efficient.
@@ -5299,10 +5418,6 @@ static int qemuStringToArgvEnv(const char *args, const char *next;
start = curr; - /* accept a space in CEPH_ARGS */ - if (STRPREFIX(curr, "CEPH_ARGS=-m ")) { - start += strlen("CEPH_ARGS=-m "); - }
While we aren't generating CEPH_ARGS in the environment any more, I don't think we should rip out the parsing code. That is, parsing should be able to handle our older output, in addition to our new output.
+++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-auth.args @@ -0,0 +1,6 @@ +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 -drive \ +file=/dev/HostVG/QEMUGuest1,if=ide,bus=0,unit=0 -drive \ +file=rbd:pool/image:id=myname:key=QVFDVm41aE82SHpGQWhBQXEwTkN2OGp0SmNJY0UrSE9CbE1RMUE=:auth_supported=cephx\;none:mon_host=mon1.example.org\:6321\;mon2.example.org\:6322\;mon3.example.org\:6322,\
That's a rather long line - I'd suggest using backslash-newline to break it up, probably at each ':' (it's not the end of the world if you go longer than 80 columns, if absolutely necessary, but life is easier when everything just fits).
+static virSecretPtr +fakeSecretLookupByUsage(virConnectPtr conn, + int usageType ATTRIBUTE_UNUSED, + const char *usageID) +{ + virSecretPtr ret = NULL; + int err; + if (STRNEQ(usageID, "mycluster_myname")) + return NULL; + err = VIR_ALLOC(ret); + if (err< 0) + return NULL; + ret->magic = VIR_SECRET_MAGIC; + ret->conn = conn; + conn->refs++; + ret->refs = 1; + ret->usageID = strdup(usageID);
Check for strdup() failure. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
From: Sage Weil <sage@newdream.net> 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. An <auth> 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 <sage@newdream.net> Signed-off-by: Josh Durgin <josh.durgin@dreamhost.com> --- Changes since v4: * fixes memory management issues * keep older rbd command line parsing and test case * check qemuAddRBDHost return values * use more efficient virBuffer functions src/qemu/qemu_command.c | 356 ++++++++++++++------ tests/qemuargv2xmltest.c | 2 + .../qemuxml2argv-disk-drive-network-rbd-auth.args | 10 + .../qemuxml2argv-disk-drive-network-rbd-auth.xml | 37 ++ ...muxml2argv-disk-drive-network-rbd-ceph-env.args | 6 + ...emuxml2argv-disk-drive-network-rbd-ceph-env.xml | 34 ++ .../qemuxml2argv-disk-drive-network-rbd.args | 7 +- tests/qemuxml2argvtest.c | 58 ++++ 8 files changed, 406 insertions(+), 104 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 create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-ceph-env.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-ceph-env.xml diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index dc92fa3..55859e2 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 <sys/utsname.h> #include <sys/stat.h> @@ -1495,6 +1496,189 @@ qemuSafeSerialParamValue(const char *value) return 0; } +static int +qemuBuildRBDString(virConnectPtr conn, + virDomainDiskDefPtr disk, + virBufferPtr opt) +{ + int i, ret = 0; + 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 = NULL; + + 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); + goto error; + } + /* qemu/librbd wants it base64 encoded */ + base64_encode_alloc(secret, secret_size, &base64); + if (!base64) { + virReportOOMError(); + goto error; + } + virBufferEscape(opt, ":", ":key=%s:auth_supported=cephx none", + base64); + VIR_FREE(base64); + } else { + qemuReportError(VIR_ERR_INTERNAL_ERROR, + _("rbd username '%s' specified but secret not found"), + disk->auth.username); + goto error; + } + } + + if (disk->nhosts > 0) { + virBufferAddLit(opt, ":mon_host="); + for (i = 0; i < disk->nhosts; ++i) { + if (i) { + virBufferAddLit(opt, "\\;"); + } + 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); + } + } + } + +cleanup: + VIR_FREE(secret); + if (sec) + virUnrefSecret(sec); + + return ret; + +error: + ret = -1; + goto cleanup; +} + +static int qemuAddRBDHost(virDomainDiskDefPtr disk, char *hostport) +{ + char *port; + + disk->nhosts++; + if (VIR_REALLOC_N(disk->hosts, disk->nhosts) < 0) + goto no_memory; + + port = strstr(hostport, "\\:"); + if (port) { + *port = '\0'; + port += 2; + disk->hosts[disk->nhosts-1].port = strdup(port); + if (!disk->hosts[disk->nhosts-1].port) + goto no_memory; + } else { + disk->hosts[disk->nhosts-1].port = strdup("6789"); + if (!disk->hosts[disk->nhosts-1].port) + goto no_memory; + } + disk->hosts[disk->nhosts-1].name = strdup(hostport); + if (!disk->hosts[disk->nhosts-1].name) + goto no_memory; + return 0; + +no_memory: + virReportOOMError(); + VIR_FREE(disk->hosts[disk->nhosts-1].port); + VIR_FREE(disk->hosts[disk->nhosts-1].name); + return -1; +} + +/* 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); + if (!options) + goto no_memory; + *p = '\0'; + } + + /* 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=")); + if (!disk->auth.username) + goto no_memory; + } + if (STRPREFIX(p, "mon_host=")) { + char *h, *sep; + + h = p + strlen("mon_host="); + while (h < e) { + for (sep = h; sep < e; ++sep) { + if (*sep == '\\' && (sep[1] == ',' || + sep[1] == ';' || + sep[1] == ' ')) { + *sep = '\0'; + sep += 2; + break; + } + } + if (qemuAddRBDHost(disk, h) < 0) { + return -1; + } + h = sep; + } + } + + p = next; + } + return 0; + +no_memory: + virReportOOMError(); + return -1; +} char * qemuBuildDriveStr(virConnectPtr conn ATTRIBUTE_UNUSED, @@ -1614,8 +1798,10 @@ qemuBuildDriveStr(virConnectPtr conn ATTRIBUTE_UNUSED, disk->hosts->name, disk->hosts->port); break; case VIR_DOMAIN_DISK_PROTOCOL_RBD: - /* TODO: set monitor hostnames */ - virBufferAsprintf(&opt, "file=rbd:%s,", disk->src); + virBufferAddLit(&opt, "file="); + if (qemuBuildRBDString(conn, disk, &opt) < 0) + goto error; + virBufferAddChar(&opt, ','); break; case VIR_DOMAIN_DISK_PROTOCOL_SHEEPDOG: if (disk->nhosts == 0) @@ -3284,8 +3470,6 @@ qemuBuildCommandLine(virConnectPtr conn, int last_good_net = -1; bool hasHwVirt = false; virCommandPtr cmd; - bool has_rbd_hosts = false; - virBuffer rbd_hosts = VIR_BUFFER_INITIALIZER; bool emitBootindex = false; int usbcontroller = 0; bool usblegacy = false; @@ -3865,7 +4049,6 @@ qemuBuildCommandLine(virConnectPtr conn, virDomainDiskDefPtr disk = def->disks[i]; int withDeviceArg = 0; bool deviceFlagMasked = false; - int j; /* Unless we have -device, then USB disks need special handling */ @@ -3923,26 +4106,6 @@ qemuBuildCommandLine(virConnectPtr conn, virCommandAddArg(cmd, optstr); VIR_FREE(optstr); - if (disk->type == VIR_DOMAIN_DISK_TYPE_NETWORK && - disk->protocol == VIR_DOMAIN_DISK_PROTOCOL_RBD) { - for (j = 0 ; j < disk->nhosts ; j++) { - if (!has_rbd_hosts) { - virBufferAddLit(&rbd_hosts, "CEPH_ARGS=-m "); - has_rbd_hosts = true; - } else { - virBufferAddLit(&rbd_hosts, ","); - } - virDomainDiskHostDefPtr host = &disk->hosts[j]; - if (host->port) { - virBufferAsprintf(&rbd_hosts, "%s:%s", - host->name, - host->port); - } else { - virBufferAdd(&rbd_hosts, host->name, -1); - } - } - } - if (!emitBootindex) bootindex = 0; else if (disk->bootIndex) @@ -3980,7 +4143,6 @@ qemuBuildCommandLine(virConnectPtr conn, char *file; const char *fmt; virDomainDiskDefPtr disk = def->disks[i]; - int j; if (disk->bus == VIR_DOMAIN_DISK_BUS_USB) { if (disk->device == VIR_DOMAIN_DISK_DEVICE_DISK) { @@ -4049,24 +4211,15 @@ qemuBuildCommandLine(virConnectPtr conn, } break; case VIR_DOMAIN_DISK_PROTOCOL_RBD: - if (virAsprintf(&file, "rbd:%s,", disk->src) < 0) { - goto no_memory; - } - for (j = 0 ; j < disk->nhosts ; j++) { - if (!has_rbd_hosts) { - virBufferAddLit(&rbd_hosts, "CEPH_ARGS=-m "); - has_rbd_hosts = true; - } else { - virBufferAddLit(&rbd_hosts, ","); - } - virDomainDiskHostDefPtr host = &disk->hosts[j]; - if (host->port) { - virBufferAsprintf(&rbd_hosts, "%s:%s", - host->name, - host->port); - } else { - virBufferAdd(&rbd_hosts, host->name, -1); + { + 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_SHEEPDOG: @@ -4095,9 +4248,6 @@ qemuBuildCommandLine(virConnectPtr conn, } } - if (has_rbd_hosts) - virCommandAddEnvBuffer(cmd, &rbd_hosts); - if (qemuCapsGet(qemuCaps, QEMU_CAPS_FSDEV)) { for (i = 0 ; i < def->nfss ; i++) { char *optstr; @@ -5268,7 +5418,6 @@ qemuBuildCommandLine(virConnectPtr conn, networkReleaseActualDevice(def->nets[i]); for (i = 0; i <= last_good_net; i++) virDomainConfNWFilterTeardown(def->nets[i]); - virBufferFreeAndReset(&rbd_hosts); virCommandFree(cmd); return NULL; } @@ -5507,7 +5656,8 @@ error: static virDomainDiskDefPtr qemuParseCommandLineDisk(virCapsPtr caps, const char *val, - int nvirtiodisk) + int nvirtiodisk, + bool old_style_ceph_args) { virDomainDiskDefPtr def = NULL; char **keywords; @@ -5582,6 +5732,9 @@ qemuParseCommandLineDisk(virCapsPtr caps, virReportOOMError(); goto cleanup; } + /* old-style CEPH_ARGS env variable is parsed later */ + if (!old_style_ceph_args && qemuParseRBDString(def) < 0) + goto cleanup; VIR_FREE(p); } else if (STRPREFIX(def->src, "sheepdog:")) { @@ -6447,6 +6600,7 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps, int nvirtiodisk = 0; qemuDomainCmdlineDefPtr cmd = NULL; virDomainDiskDefPtr disk = NULL; + const char *ceph_args = qemuFindEnv(progenv, "CEPH_ARGS"); if (pidfile) *pidfile = NULL; @@ -6701,7 +6855,9 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps, disk->src = NULL; break; case VIR_DOMAIN_DISK_PROTOCOL_RBD: - /* handled later since the hosts for all disks are in CEPH_ARGS */ + /* old-style CEPH_ARGS env variable is parsed later */ + if (!ceph_args && qemuParseRBDString(disk) < 0) + goto error; break; case VIR_DOMAIN_DISK_PROTOCOL_SHEEPDOG: /* disk->src must be [vdiname] or [host]:[port]:[vdiname] */ @@ -6901,7 +7057,8 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps, } } else if (STREQ(arg, "-drive")) { WANT_VALUE(); - if (!(disk = qemuParseCommandLineDisk(caps, val, nvirtiodisk))) + if (!(disk = qemuParseCommandLineDisk(caps, val, nvirtiodisk, + ceph_args != NULL))) goto error; if (VIR_REALLOC_N(def->disks, def->ndisks+1) < 0) goto no_memory; @@ -7040,66 +7197,63 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps, } #undef WANT_VALUE - if (def->ndisks > 0) { - const char *ceph_args = qemuFindEnv(progenv, "CEPH_ARGS"); - if (ceph_args) { - char *hosts, *port, *saveptr = NULL, *token; - virDomainDiskDefPtr first_rbd_disk = NULL; - for (i = 0 ; i < def->ndisks ; i++) { - if (def->disks[i]->type == VIR_DOMAIN_DISK_TYPE_NETWORK && - def->disks[i]->protocol == VIR_DOMAIN_DISK_PROTOCOL_RBD) { - first_rbd_disk = def->disks[i]; - break; - } + if (def->ndisks > 0 && ceph_args) { + char *hosts, *port, *saveptr = NULL, *token; + virDomainDiskDefPtr first_rbd_disk = NULL; + for (i = 0 ; i < def->ndisks ; i++) { + if (def->disks[i]->type == VIR_DOMAIN_DISK_TYPE_NETWORK && + def->disks[i]->protocol == VIR_DOMAIN_DISK_PROTOCOL_RBD) { + first_rbd_disk = def->disks[i]; + break; } + } - if (!first_rbd_disk) { - qemuReportError(VIR_ERR_INTERNAL_ERROR, - _("CEPH_ARGS was set without an rbd disk")); - goto error; - } + if (!first_rbd_disk) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, + _("CEPH_ARGS was set without an rbd disk")); + goto error; + } - /* CEPH_ARGS should be: -m host1[:port1][,host2[:port2]]... */ - if (!STRPREFIX(ceph_args, "-m ")) { - qemuReportError(VIR_ERR_INTERNAL_ERROR, - _("could not parse CEPH_ARGS '%s'"), ceph_args); - goto error; - } - hosts = strdup(strchr(ceph_args, ' ') + 1); - if (!hosts) + /* CEPH_ARGS should be: -m host1[:port1][,host2[:port2]]... */ + if (!STRPREFIX(ceph_args, "-m ")) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, + _("could not parse CEPH_ARGS '%s'"), ceph_args); + goto error; + } + hosts = strdup(strchr(ceph_args, ' ') + 1); + if (!hosts) + goto no_memory; + first_rbd_disk->nhosts = 0; + token = strtok_r(hosts, ",", &saveptr); + while (token != NULL) { + if (VIR_REALLOC_N(first_rbd_disk->hosts, first_rbd_disk->nhosts + 1) < 0) { + VIR_FREE(hosts); goto no_memory; - first_rbd_disk->nhosts = 0; - token = strtok_r(hosts, ",", &saveptr); - while (token != NULL) { - if (VIR_REALLOC_N(first_rbd_disk->hosts, first_rbd_disk->nhosts + 1) < 0) { - VIR_FREE(hosts); - goto no_memory; - } - port = strchr(token, ':'); - if (port) { - *port++ = '\0'; - port = strdup(port); - if (!port) { - VIR_FREE(hosts); - goto no_memory; - } - } - first_rbd_disk->hosts[first_rbd_disk->nhosts].port = port; - first_rbd_disk->hosts[first_rbd_disk->nhosts].name = strdup(token); - if (!first_rbd_disk->hosts[first_rbd_disk->nhosts].name) { + } + port = strchr(token, ':'); + if (port) { + *port++ = '\0'; + port = strdup(port); + if (!port) { VIR_FREE(hosts); goto no_memory; } - first_rbd_disk->nhosts++; - token = strtok_r(NULL, ",", &saveptr); } - VIR_FREE(hosts); - - if (first_rbd_disk->nhosts == 0) { - qemuReportError(VIR_ERR_INTERNAL_ERROR, - _("found no rbd hosts in CEPH_ARGS '%s'"), ceph_args); - goto error; + first_rbd_disk->hosts[first_rbd_disk->nhosts].port = port; + first_rbd_disk->hosts[first_rbd_disk->nhosts].name = strdup(token); + if (!first_rbd_disk->hosts[first_rbd_disk->nhosts].name) { + VIR_FREE(hosts); + goto no_memory; } + first_rbd_disk->nhosts++; + token = strtok_r(NULL, ",", &saveptr); + } + VIR_FREE(hosts); + + if (first_rbd_disk->nhosts == 0) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, + _("found no rbd hosts in CEPH_ARGS '%s'"), ceph_args); + goto error; } } diff --git a/tests/qemuargv2xmltest.c b/tests/qemuargv2xmltest.c index 807a771..a20df06 100644 --- a/tests/qemuargv2xmltest.c +++ b/tests/qemuargv2xmltest.c @@ -173,6 +173,8 @@ mymain(void) DO_TEST("disk-drive-cache-unsafe"); DO_TEST("disk-drive-network-nbd"); DO_TEST("disk-drive-network-rbd"); + /* older format using CEPH_ARGS env var */ + DO_TEST("disk-drive-network-rbd-ceph-env"); DO_TEST("disk-drive-network-sheepdog"); DO_TEST("disk-usb"); DO_TEST("graphics-vnc"); diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-auth.args b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-auth.args new file mode 100644 index 0000000..1500672 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-auth.args @@ -0,0 +1,10 @@ +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 -drive \ +file=/dev/HostVG/QEMUGuest1,if=ide,bus=0,unit=0 -drive \ +file=rbd:pool/image:\ +id=myname:\ +key=QVFDVm41aE82SHpGQWhBQXEwTkN2OGp0SmNJY0UrSE9CbE1RMUE=:\ +auth_supported=cephx none:\ +mon_host=mon1.example.org\:6321\;mon2.example.org\:6322\;mon3.example.org\:6322,\ +if=virtio,format=raw -net none -serial none -parallel none -usb diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-auth.xml b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-auth.xml new file mode 100644 index 0000000..88f7f7a --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-auth.xml @@ -0,0 +1,37 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory>219136</memory> + <currentMemory>219136</currentMemory> + <vcpu>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' unit='0'/> + </disk> + <disk type='network' device='disk'> + <driver name='qemu' type='raw'/> + <auth username='myname'> + <secret type='ceph' usage='mycluster_myname'/> + </auth> + <source protocol='rbd' name='pool/image'> + <host name='mon1.example.org' port='6321'/> + <host name='mon2.example.org' port='6322'/> + <host name='mon3.example.org' port='6322'/> + </source> + <target dev='vda' bus='virtio'/> + </disk> + <controller type='ide' index='0'/> + <memballoon model='virtio'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-ceph-env.args b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-ceph-env.args new file mode 100644 index 0000000..3ab1219 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-ceph-env.args @@ -0,0 +1,6 @@ +LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test CEPH_ARGS=-m \ +mon1.example.org:6321,mon2.example.org:6322,mon3.example.org:6322 \ +/usr/bin/qemu -S -M pc -m 214 -smp 1 -nographic -monitor \ +unix:/tmp/test-monitor,server,nowait -no-acpi -boot c -drive \ +file=/dev/HostVG/QEMUGuest1,if=ide,bus=0,unit=0 -drive file=rbd:pool/image,\ +if=virtio,format=raw -net none -serial none -parallel none -usb diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-ceph-env.xml b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-ceph-env.xml new file mode 100644 index 0000000..e920db1 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-ceph-env.xml @@ -0,0 +1,34 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory>219136</memory> + <currentMemory>219136</currentMemory> + <vcpu>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' unit='0'/> + </disk> + <disk type='network' device='disk'> + <driver name='qemu' type='raw'/> + <source protocol='rbd' name='pool/image'> + <host name='mon1.example.org' port='6321'/> + <host name='mon2.example.org' port='6322'/> + <host name='mon3.example.org' port='6322'/> + </source> + <target dev='vda' bus='virtio'/> + </disk> + <controller type='ide' index='0'/> + <memballoon model='virtio'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd.args b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd.args index 3ab1219..706ba89 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd.args @@ -1,6 +1,7 @@ -LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test CEPH_ARGS=-m \ -mon1.example.org:6321,mon2.example.org:6322,mon3.example.org:6322 \ +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 -drive \ -file=/dev/HostVG/QEMUGuest1,if=ide,bus=0,unit=0 -drive file=rbd:pool/image,\ +file=/dev/HostVG/QEMUGuest1,if=ide,bus=0,unit=0 -drive \ +file=rbd:pool/image:\ +mon_host=mon1.example.org\:6321\;mon2.example.org\:6322\;mon3.example.org\:6322,\ if=virtio,format=raw -net none -serial none -parallel none -usb diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 4d6db01..e7c1ffd 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -12,6 +12,7 @@ # include "internal.h" # include "testutils.h" +# include "util/memory.h" # include "qemu/qemu_capabilities.h" # include "qemu/qemu_command.h" # include "qemu/qemu_domain.h" @@ -23,6 +24,60 @@ static const char *abs_top_srcdir; static struct qemud_driver driver; +static unsigned char * +fakeSecretGetValue(virSecretPtr obj ATTRIBUTE_UNUSED, + size_t *value_size, + unsigned int fakeflags ATTRIBUTE_UNUSED, + unsigned int internalFlags ATTRIBUTE_UNUSED) +{ + char *secret = strdup("AQCVn5hO6HzFAhAAq0NCv8jtJcIcE+HOBlMQ1A"); + *value_size = strlen(secret); + return (unsigned char *) secret; +} + +static virSecretPtr +fakeSecretLookupByUsage(virConnectPtr conn, + int usageType ATTRIBUTE_UNUSED, + const char *usageID) +{ + virSecretPtr ret = NULL; + int err; + if (STRNEQ(usageID, "mycluster_myname")) + return NULL; + err = VIR_ALLOC(ret); + if (err < 0) + return NULL; + ret->magic = VIR_SECRET_MAGIC; + ret->refs = 1; + ret->usageID = strdup(usageID); + if (!ret->usageID) + return NULL; + ret->conn = conn; + conn->refs++; + return ret; +} + +static int +fakeSecretClose(virConnectPtr conn ATTRIBUTE_UNUSED) +{ + return 0; +} + +static virSecretDriver fakeSecretDriver = { + .name = "fake_secret", + .open = NULL, + .close = fakeSecretClose, + .numOfSecrets = NULL, + .listSecrets = NULL, + .lookupByUUID = NULL, + .lookupByUsage = fakeSecretLookupByUsage, + .defineXML = NULL, + .getXMLDesc = NULL, + .setValue = NULL, + .getValue = fakeSecretGetValue, + .undefine = NULL, +}; + static int testCompareXMLToArgvFiles(const char *xml, const char *cmdline, virBitmapPtr extraFlags, @@ -44,6 +99,7 @@ static int testCompareXMLToArgvFiles(const char *xml, if (!(conn = virGetConnect())) goto fail; + conn->secretDriver = &fakeSecretDriver; len = virtTestLoadFile(cmdline, &expectargv); if (len < 0) @@ -357,6 +413,8 @@ mymain(void) QEMU_CAPS_DRIVE, QEMU_CAPS_DRIVE_FORMAT); DO_TEST("disk-drive-network-sheepdog", false, QEMU_CAPS_DRIVE, QEMU_CAPS_DRIVE_FORMAT); + DO_TEST("disk-drive-network-rbd-auth", false, + QEMU_CAPS_DRIVE, QEMU_CAPS_DRIVE_FORMAT); DO_TEST("disk-drive-no-boot", false, QEMU_CAPS_DRIVE, QEMU_CAPS_DEVICE, QEMU_CAPS_BOOTINDEX); DO_TEST("disk-usb", false, NONE); -- 1.7.1
On 10/31/2011 07:29 PM, Josh Durgin wrote:
From: Sage Weil <sage@newdream.net>
Sorry for letting my review of this slip 2 weeks.
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.
An <auth> 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 <sage@newdream.net> Signed-off-by: Josh Durgin <josh.durgin@dreamhost.com> ---
Changes since v4: * fixes memory management issues * keep older rbd command line parsing and test case * check qemuAddRBDHost return values * use more efficient virBuffer functions
Looks like you got all my review points. ACK and pushed, although I do have some questions that may deserve followup patches:
+static int +qemuBuildRBDString(virConnectPtr conn, + virDomainDiskDefPtr disk, + virBufferPtr opt) +{ + int i, ret = 0; + 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);
This results in ambiguous output if disk->auth.username can end in a single backslash (since then, you would have \: when combined with the next part of the option, making it look like the next ":mon_host=" option is instead a continuation of the ":id=" username). Should we be escaping backslash as well as colon? Or should virBufferEscape be taught to always escape backslash in addition to whatever characters were passed in to its 'toescape' argument?
+ if (sec) { + char *base64 = NULL; + + 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); + goto error; + } + /* qemu/librbd wants it base64 encoded */ + base64_encode_alloc(secret, secret_size, &base64); + if (!base64) { + virReportOOMError(); + goto error; + } + virBufferEscape(opt, ":", ":key=%s:auth_supported=cephx none", + base64); + VIR_FREE(base64);
The command line that we pass to qemu gets logged. But what happens if the secret was marked as ephemeral - could we be violating the premise of not exposing passwords to too broad an audience? Or are we already safe in that the log entries created by virCommand can only be exposed to users that already can get at the secret information by other means? Maybe this means we should we be adding capabilities into virCommand to prevent the logging of the actual secret (whether base64-encoded or otherwise), and instead log an alternate string? That is, should virCommand be tracking parallel argv arrays; the real array passed to exec() but never logged, and the alternate array (normally matching the real one, but which can differ in this particular case of passing an argument that contains a password)? -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 11/15/2011 04:05 PM, Eric Blake wrote:
On 10/31/2011 07:29 PM, Josh Durgin wrote:
From: Sage Weil<sage@newdream.net>
Sorry for letting my review of this slip 2 weeks.
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.
An<auth> 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<sage@newdream.net> Signed-off-by: Josh Durgin<josh.durgin@dreamhost.com> ---
Changes since v4: * fixes memory management issues * keep older rbd command line parsing and test case * check qemuAddRBDHost return values * use more efficient virBuffer functions
Looks like you got all my review points.
ACK and pushed, although I do have some questions that may deserve followup patches:
+static int +qemuBuildRBDString(virConnectPtr conn, + virDomainDiskDefPtr disk, + virBufferPtr opt) +{ + int i, ret = 0; + 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);
This results in ambiguous output if disk->auth.username can end in a single backslash (since then, you would have \: when combined with the next part of the option, making it look like the next ":mon_host=" option is instead a continuation of the ":id=" username). Should we be escaping backslash as well as colon? Or should virBufferEscape be taught to always escape backslash in addition to whatever characters were passed in to its 'toescape' argument?
Escaping backslashes wouldn't hurt, but these usernames aren't expected to have backslashes in them (they're genericNames in the xml schema).
+ if (sec) { + char *base64 = NULL; + + 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); + goto error; + } + /* qemu/librbd wants it base64 encoded */ + base64_encode_alloc(secret, secret_size,&base64); + if (!base64) { + virReportOOMError(); + goto error; + } + virBufferEscape(opt, ":", ":key=%s:auth_supported=cephx none", + base64); + VIR_FREE(base64);
The command line that we pass to qemu gets logged. But what happens if the secret was marked as ephemeral - could we be violating the premise of not exposing passwords to too broad an audience? Or are we already safe in that the log entries created by virCommand can only be exposed to users that already can get at the secret information by other means?
The secret can be read from the command line of the running process, which is even less secure than the log. I'm working on passing the secret via the qemu monitor instead of the command line, which will avoid both issues.
Maybe this means we should we be adding capabilities into virCommand to prevent the logging of the actual secret (whether base64-encoded or otherwise), and instead log an alternate string? That is, should virCommand be tracking parallel argv arrays; the real array passed to exec() but never logged, and the alternate array (normally matching the real one, but which can differ in this particular case of passing an argument that contains a password)?
On 11/15/2011 06:37 PM, Josh Durgin wrote:
The command line that we pass to qemu gets logged. But what happens if the secret was marked as ephemeral - could we be violating the premise of not exposing passwords to too broad an audience? Or are we already safe in that the log entries created by virCommand can only be exposed to users that already can get at the secret information by other means?
The secret can be read from the command line of the running process, which is even less secure than the log. I'm working on passing the secret via the qemu monitor instead of the command line, which will avoid both issues.
Maybe this means we should we be adding capabilities into virCommand to prevent the logging of the actual secret (whether base64-encoded or otherwise), and instead log an alternate string? That is, should virCommand be tracking parallel argv arrays; the real array passed to exec() but never logged, and the alternate array (normally matching the real one, but which can differ in this particular case of passing an argument that contains a password)?
Given your arguments (that ps can read argv of qemu, even if we hid it from libvirt logs, and that we will be moving to a monitor command as soon as qemu supports one), I see no reason to hack up virCommand to support alternate log output. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Tue, Nov 15, 2011 at 05:05:27PM -0700, Eric Blake wrote:
On 10/31/2011 07:29 PM, Josh Durgin wrote:
From: Sage Weil <sage@newdream.net> + if (sec) { + char *base64 = NULL; + + 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); + goto error; + } + /* qemu/librbd wants it base64 encoded */ + base64_encode_alloc(secret, secret_size, &base64); + if (!base64) { + virReportOOMError(); + goto error; + } + virBufferEscape(opt, ":", ":key=%s:auth_supported=cephx none", + base64); + VIR_FREE(base64);
The command line that we pass to qemu gets logged. But what happens if the secret was marked as ephemeral - could we be violating the premise of not exposing passwords to too broad an audience? Or are we already safe in that the log entries created by virCommand can only be exposed to users that already can get at the secret information by other means?
Maybe this means we should we be adding capabilities into virCommand to prevent the logging of the actual secret (whether base64-encoded or otherwise), and instead log an alternate string? That is, should virCommand be tracking parallel argv arrays; the real array passed to exec() but never logged, and the alternate array (normally matching the real one, but which can differ in this particular case of passing an argument that contains a password)?
The passing of secrets on the command line is just a temporary hack we're doing to prove the overall handling of passwords for Ceph. The real plan is to set them via monitor command in QEMU, but we're just waiting for some QEMU work before changing libvirt todo that. 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 :|
participants (3)
-
Daniel P. Berrange -
Eric Blake -
Josh Durgin