[libvirt] [RFC PATCH v3 4/4] qemu/rbd: improve rbd device specification
Daniel P. Berrange
berrange at redhat.com
Thu Oct 27 08:38:23 UTC 2011
On Thu, Oct 20, 2011 at 11:01:27AM -0700, Josh Durgin wrote:
> From: Sage Weil <sage at 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 at newdream.net>
> Signed-off-by: Josh Durgin <josh.durgin at 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 :|
More information about the libvir-list
mailing list