[libvirt] [PATCH 0/5 v2] Improve Ceph Qemu+RBD support
The current support for qemu and Ceph RBD (rados block device) has two main deficiencies: authentication doesn't work, and it relies on environment variables (which don't work with latest upstream). This patch set addresses both those problems. The first two patches update the xml schemas and conf to add a Ceph secret type and to specify authentication information along with the rbd disk. The next two patches make some libvirt changes. We pass virConnectPtr down into the Domain{Attach,Detach} methods (needed to access secrets while building the qemu command), and add a helper that will escape arbitrary characters. The final patch replaces the current RBD qemu code and uses the new conf info to do authentication properly. (We still need to make a change there to avoid having the authentication key show up on qemu command line; I'll clean that up shortly.) Comments on this approach? Thanks! sage Changes from v1: update docs/schemas/{domain,secret}.rng Sage Weil (5): secret: add Ceph secret type storage: add authId, authDomain to virDomainDiskDef qemu: pass virConnectPtr into Domain{Attach,Detach}* buf: implement generic virBufferEscape qemu/rbd: improve rbd device specification docs/schemas/domain.rng | 6 + docs/schemas/secret.rng | 17 ++ include/libvirt/libvirt.h.in | 3 + src/conf/domain_conf.c | 43 +++- src/conf/domain_conf.h | 2 + src/conf/secret_conf.c | 45 ++++- src/conf/secret_conf.h | 1 + src/libvirt_private.syms | 1 + src/qemu/qemu_command.c | 273 +++++++++++--------- src/qemu/qemu_command.h | 3 +- src/qemu/qemu_driver.c | 17 +- src/qemu/qemu_hotplug.c | 15 +- src/qemu/qemu_hotplug.h | 9 +- src/secret/secret_driver.c | 8 + src/util/buf.c | 33 ++- src/util/buf.h | 1 + .../qemuxml2argv-disk-drive-network-rbd.args | 6 +- .../qemuxml2argv-disk-drive-network-rbd.xml | 1 + 18 files changed, 328 insertions(+), 156 deletions(-) -- 1.7.4.1
From 498cd06b76bbb4415a2f81f9d169f267ff99329c Mon Sep 17 00:00:00 2001 From: Sage Weil <sage@newdream.net> Date: Thu, 15 Sep 2011 13:47:40 -0700 Subject: [PATCH 0/5] Improve Ceph Qemu+RBD support
The current support for qemu and Ceph RBD (rados block device) has two main deficiencies: authentication doesn't work, and it relies on environment variables (which don't work with latest upstream). This patch set addresses both those problems, while trying to integrate as cleanly as possible with the rest of libvirt. The first few patches make some changes to libvirt itself: adding a CEPH secret type (for Ceph/RBD authentication), adding authentication fields to the XML schema, passing the virConnectPtr into the Domain{Attach,Detach} methods (needed to access secrets while building the qemu command), a helper that will escape arbitrary characters, and finally a patch that replaces the current RBD qemu code. Comments on this approach? Thanks! sage Sage Weil (5): secret: add Ceph secret type storage: add authId, authDomain to virDomainDiskDef qemu: pass virConnectPtr into Domain{Attach,Detach}* buf: implement generic virBufferEscape qemu/rbd: improve rbd device specification include/libvirt/libvirt.h.in | 3 + src/conf/domain_conf.c | 43 +++- src/conf/domain_conf.h | 2 + src/conf/secret_conf.c | 45 ++++- src/conf/secret_conf.h | 1 + src/libvirt_private.syms | 1 + src/qemu/qemu_command.c | 273 +++++++++++--------- src/qemu/qemu_command.h | 3 +- src/qemu/qemu_driver.c | 17 +- src/qemu/qemu_hotplug.c | 15 +- src/qemu/qemu_hotplug.h | 9 +- src/secret/secret_driver.c | 8 + src/util/buf.c | 33 ++- src/util/buf.h | 1 + .../qemuxml2argv-disk-drive-network-rbd.args | 6 +- .../qemuxml2argv-disk-drive-network-rbd.xml | 1 + 16 files changed, 305 insertions(+), 156 deletions(-) -- 1.7.4.1
Add a new secret type to store a Ceph authentication key. The ceph_id field contains the name of the key (e.g. 'admin' for the ceph superuser). Signed-off-by: Sage Weil <sage@newdream.net> --- docs/schemas/secret.rng | 17 +++++++++++++++ include/libvirt/libvirt.h.in | 3 ++ src/conf/secret_conf.c | 45 +++++++++++++++++++++++++++++++++++++++++- src/conf/secret_conf.h | 1 + src/secret/secret_driver.c | 8 +++++++ 5 files changed, 73 insertions(+), 1 deletions(-) diff --git a/docs/schemas/secret.rng b/docs/schemas/secret.rng index 80270ae..c3da8b3 100644 --- a/docs/schemas/secret.rng +++ b/docs/schemas/secret.rng @@ -37,6 +37,7 @@ <element name='usage'> <choice> <ref name='usagevolume'/> + <ref name='cephauth'/> <!-- More choices later --> </choice> </element> @@ -54,6 +55,22 @@ </element> </define> + <define name='cephauth'> + <attribute name='type'> + <value>ceph</value> + </attribute> + <element name='auth'> + <attribute name='id'> + <text/> + </attribute> + <optional> + <attribute name='domain'> + <text/> + </attribute> + </optional> + </element> + </define> + <define name="UUID"> <choice> <data type="string"> diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index b1bda31..51fd044 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -2257,7 +2257,10 @@ typedef virSecret *virSecretPtr; typedef enum { VIR_SECRET_USAGE_TYPE_NONE = 0, VIR_SECRET_USAGE_TYPE_VOLUME = 1, + VIR_SECRET_USAGE_TYPE_CEPH = 2, /* Expect more owner types later... */ + + VIR_SECRET_USAGE_TYPE_LAST } virSecretUsageType; virConnectPtr virSecretGetConnect (virSecretPtr secret); diff --git a/src/conf/secret_conf.c b/src/conf/secret_conf.c index 105afbe..8f11a51 100644 --- a/src/conf/secret_conf.c +++ b/src/conf/secret_conf.c @@ -35,7 +35,8 @@ #define VIR_FROM_THIS VIR_FROM_SECRET -VIR_ENUM_IMPL(virSecretUsageType, VIR_SECRET_USAGE_TYPE_VOLUME + 1, "none", "volume") +VIR_ENUM_IMPL(virSecretUsageType, VIR_SECRET_USAGE_TYPE_LAST, + "none", "volume", "ceph") void virSecretDefFree(virSecretDefPtr def) @@ -52,6 +53,10 @@ virSecretDefFree(virSecretDefPtr def) VIR_FREE(def->usage.volume); break; + case VIR_SECRET_USAGE_TYPE_CEPH: + VIR_FREE(def->usage.authIdDomain); + break; + default: VIR_ERROR(_("unexpected secret usage type %d"), def->usage_type); break; @@ -65,6 +70,8 @@ virSecretDefParseUsage(xmlXPathContextPtr ctxt, { char *type_str; int type; + char *authId, *authDomain; + int ret; type_str = virXPathString("string(./usage/@type)", ctxt); if (type_str == NULL) { @@ -94,6 +101,27 @@ virSecretDefParseUsage(xmlXPathContextPtr ctxt, } break; + case VIR_SECRET_USAGE_TYPE_CEPH: + authId = virXPathString("string(./usage/auth/@id)", ctxt); + if (!authId) { + virSecretReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("ceph usage specified, but auth id is missing")); + return -1; + } + authDomain = virXPathString("string(./usage/auth/@domain)", ctxt); + if (!authDomain) { + VIR_FREE(authId); + virSecretReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("ceph usage specified, but auth domain is missing")); + return -1; + } + ret = virAlloc(&def->usage.authIdDomain, strlen(authId) + + strlen(authDomain) + 2); + sprintf(def->usage.authIdDomain, "%s/%s", authId, authDomain); + VIR_FREE(authId); + VIR_FREE(authDomain); + break; + default: virSecretReportError(VIR_ERR_INTERNAL_ERROR, _("unexpected secret usage type %d"), @@ -220,6 +248,9 @@ virSecretDefFormatUsage(virBufferPtr buf, const virSecretDefPtr def) { const char *type; + char *p; + char idAuth[80]; + int len; type = virSecretUsageTypeTypeToString(def->usage_type); if (type == NULL) { @@ -239,6 +270,18 @@ virSecretDefFormatUsage(virBufferPtr buf, def->usage.volume); break; + case VIR_SECRET_USAGE_TYPE_CEPH: + if (def->usage.authIdDomain != NULL) { + p = strchr(def->usage.authIdDomain, '/'); + len = p - def->usage.authIdDomain; + strncpy(idAuth, def->usage.authIdDomain, len); + idAuth[len] = '\0'; + p++; + virBufferEscapeString(buf, " <auth id='%s'", idAuth); + virBufferEscapeString(buf, " domain='%s'/>\n", p); + } + break; + default: virSecretReportError(VIR_ERR_INTERNAL_ERROR, _("unexpected secret usage type %d"), diff --git a/src/conf/secret_conf.h b/src/conf/secret_conf.h index 4b47c52..294e7ab 100644 --- a/src/conf/secret_conf.h +++ b/src/conf/secret_conf.h @@ -42,6 +42,7 @@ struct _virSecretDef { int usage_type; union { char *volume; /* May be NULL */ + char *authIdDomain; } usage; }; diff --git a/src/secret/secret_driver.c b/src/secret/secret_driver.c index 59dc687..7ea8a49 100644 --- a/src/secret/secret_driver.c +++ b/src/secret/secret_driver.c @@ -144,6 +144,11 @@ secretFindByUsage(virSecretDriverStatePtr driver, int usageType, const char *usa if (STREQ(s->def->usage.volume, usageID)) return s; break; + + case VIR_SECRET_USAGE_TYPE_CEPH: + if (STREQ(s->def->usage.authIdDomain, usageID)) + return s; + break; } } return NULL; @@ -607,6 +612,9 @@ secretUsageIDForDef(virSecretDefPtr def) case VIR_SECRET_USAGE_TYPE_VOLUME: return def->usage.volume; + case VIR_SECRET_USAGE_TYPE_CEPH: + return def->usage.authIdDomain; + default: return NULL; } -- 1.7.4.1
On Mon, Sep 19, 2011 at 09:13:39PM -0700, Sage Weil wrote:
Add a new secret type to store a Ceph authentication key. The ceph_id field contains the name of the key (e.g. 'admin' for the ceph superuser).
Signed-off-by: Sage Weil <sage@newdream.net> --- docs/schemas/secret.rng | 17 +++++++++++++++ include/libvirt/libvirt.h.in | 3 ++ src/conf/secret_conf.c | 45 +++++++++++++++++++++++++++++++++++++++++- src/conf/secret_conf.h | 1 + src/secret/secret_driver.c | 8 +++++++ 5 files changed, 73 insertions(+), 1 deletions(-)
diff --git a/docs/schemas/secret.rng b/docs/schemas/secret.rng index 80270ae..c3da8b3 100644 --- a/docs/schemas/secret.rng +++ b/docs/schemas/secret.rng @@ -37,6 +37,7 @@ <element name='usage'> <choice> <ref name='usagevolume'/> + <ref name='cephauth'/> <!-- More choices later --> </choice> </element> @@ -54,6 +55,22 @@ </element> </define>
+ <define name='cephauth'> + <attribute name='type'> + <value>ceph</value> + </attribute> + <element name='auth'> + <attribute name='id'> + <text/> + </attribute> + <optional> + <attribute name='domain'> + <text/> + </attribute>
Here I would expect just <element name='domain'> <text/> </element>
+ </optional> + </element> + </define> + <define name="UUID"> <choice> <data type="string"> diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index b1bda31..51fd044 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -2257,7 +2257,10 @@ typedef virSecret *virSecretPtr; typedef enum { VIR_SECRET_USAGE_TYPE_NONE = 0, VIR_SECRET_USAGE_TYPE_VOLUME = 1, + VIR_SECRET_USAGE_TYPE_CEPH = 2, /* Expect more owner types later... */ + + VIR_SECRET_USAGE_TYPE_LAST } virSecretUsageType;
virConnectPtr virSecretGetConnect (virSecretPtr secret); diff --git a/src/conf/secret_conf.c b/src/conf/secret_conf.c index 105afbe..8f11a51 100644 --- a/src/conf/secret_conf.c +++ b/src/conf/secret_conf.c @@ -35,7 +35,8 @@
#define VIR_FROM_THIS VIR_FROM_SECRET
-VIR_ENUM_IMPL(virSecretUsageType, VIR_SECRET_USAGE_TYPE_VOLUME + 1, "none", "volume") +VIR_ENUM_IMPL(virSecretUsageType, VIR_SECRET_USAGE_TYPE_LAST, + "none", "volume", "ceph")
void virSecretDefFree(virSecretDefPtr def) @@ -52,6 +53,10 @@ virSecretDefFree(virSecretDefPtr def) VIR_FREE(def->usage.volume); break;
+ case VIR_SECRET_USAGE_TYPE_CEPH: + VIR_FREE(def->usage.authIdDomain); + break; + default: VIR_ERROR(_("unexpected secret usage type %d"), def->usage_type); break; @@ -65,6 +70,8 @@ virSecretDefParseUsage(xmlXPathContextPtr ctxt, { char *type_str; int type; + char *authId, *authDomain; + int ret;
type_str = virXPathString("string(./usage/@type)", ctxt); if (type_str == NULL) { @@ -94,6 +101,27 @@ virSecretDefParseUsage(xmlXPathContextPtr ctxt, } break;
+ case VIR_SECRET_USAGE_TYPE_CEPH: + authId = virXPathString("string(./usage/auth/@id)", ctxt); + if (!authId) { + virSecretReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("ceph usage specified, but auth id is missing")); + return -1; + } + authDomain = virXPathString("string(./usage/auth/@domain)", ctxt); + if (!authDomain) { + VIR_FREE(authId); + virSecretReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("ceph usage specified, but auth domain is missing")); + return -1; + } + ret = virAlloc(&def->usage.authIdDomain, strlen(authId) + + strlen(authDomain) + 2); + sprintf(def->usage.authIdDomain, "%s/%s", authId, authDomain); + VIR_FREE(authId); + VIR_FREE(authDomain); + break;
...which simplifies this to just case VIR_SECRET_USAGE_TYPE_CEPH: def->usage.volume = virXPathString("string(./usage/domain)", ctxt); if (!def->usage.volume) { virSecretReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Ceph usage specified, but volume domain is missing")); return -1; } break;
+ default: virSecretReportError(VIR_ERR_INTERNAL_ERROR, _("unexpected secret usage type %d"), @@ -220,6 +248,9 @@ virSecretDefFormatUsage(virBufferPtr buf, const virSecretDefPtr def) { const char *type; + char *p; + char idAuth[80]; + int len;
type = virSecretUsageTypeTypeToString(def->usage_type); if (type == NULL) { @@ -239,6 +270,18 @@ virSecretDefFormatUsage(virBufferPtr buf, def->usage.volume); break;
+ case VIR_SECRET_USAGE_TYPE_CEPH: + if (def->usage.authIdDomain != NULL) { + p = strchr(def->usage.authIdDomain, '/'); + len = p - def->usage.authIdDomain; + strncpy(idAuth, def->usage.authIdDomain, len); + idAuth[len] = '\0'; + p++; + virBufferEscapeString(buf, " <auth id='%s'", idAuth); + virBufferEscapeString(buf, " domain='%s'/>\n", p); + } + break;
Likewise this to just virBufferEscapeString(buf, " <domain>%s</domain>\n", def->usage.authIdDomain); 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 :|
Add additional fields to let you specify the how to authenticate with a network disk type. The authId is the name to authenticate as, and the authDomain optionally describes the domain that user exists in. The latter allows us to locate a secret in using the libvirt secrets API, as the user is may not unique if libvirt is talking to multiple backend clusters. Signed-off-by: Sage Weil <sage@newdream.net> --- docs/schemas/domain.rng | 6 ++++++ src/conf/domain_conf.c | 43 ++++++++++++++++++++++++++++++++++--------- src/conf/domain_conf.h | 2 ++ 3 files changed, 42 insertions(+), 9 deletions(-) diff --git a/docs/schemas/domain.rng b/docs/schemas/domain.rng index 6ccbeed..3574f03 100644 --- a/docs/schemas/domain.rng +++ b/docs/schemas/domain.rng @@ -736,6 +736,12 @@ </attribute> <optional> <attribute name="name"/> + <element name="auth"> + <attribute name="id"/> + <optional> + <attribute name="domain"/> + </optional> + </element> </optional> <zeroOrMore> <element name="host"> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 010ce57..5b80a9e 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2066,7 +2066,7 @@ virDomainDiskDefParseXML(virCapsPtr caps, unsigned int flags) { virDomainDiskDefPtr def; - xmlNodePtr cur, host; + xmlNodePtr cur, child; char *type = NULL; char *device = NULL; char *driverName = NULL; @@ -2084,6 +2084,8 @@ virDomainDiskDefParseXML(virCapsPtr caps, char *devaddr = NULL; virStorageEncryptionPtr encryption = NULL; char *serial = NULL; + char *authId = NULL; + char *authDomain = NULL; if (VIR_ALLOC(def) < 0) { virReportOOMError(); @@ -2137,10 +2139,10 @@ virDomainDiskDefParseXML(virCapsPtr caps, _("missing name for disk source")); goto error; } - host = cur->children; - while (host != NULL) { - if (host->type == XML_ELEMENT_NODE && - xmlStrEqual(host->name, BAD_CAST "host")) { + child = cur->children; + while (child != NULL) { + if (child->type == XML_ELEMENT_NODE && + xmlStrEqual(child->name, BAD_CAST "host")) { if (VIR_REALLOC_N(hosts, nhosts + 1) < 0) { virReportOOMError(); goto error; @@ -2149,20 +2151,30 @@ virDomainDiskDefParseXML(virCapsPtr caps, hosts[nhosts].port = NULL; nhosts++; - hosts[nhosts - 1].name = virXMLPropString(host, "name"); + hosts[nhosts - 1].name = virXMLPropString(child, "name"); if (!hosts[nhosts - 1].name) { virDomainReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("missing name for host")); goto error; } - hosts[nhosts - 1].port = virXMLPropString(host, "port"); + hosts[nhosts - 1].port = virXMLPropString(child, "port"); if (!hosts[nhosts - 1].port) { virDomainReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("missing port for host")); goto error; } } - host = host->next; + if (child->type == XML_ELEMENT_NODE && + xmlStrEqual(child->name, BAD_CAST "auth")) { + authId = virXMLPropString(child, "id"); + if (!authId) { + virDomainReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("missing id for auth")); + goto error; + } + authDomain = virXMLPropString(child, "domain"); + } + child = child->next; } break; default: @@ -2373,6 +2385,10 @@ virDomainDiskDefParseXML(virCapsPtr caps, hosts = NULL; def->nhosts = nhosts; nhosts = 0; + def->authId = authId; + authId = NULL; + def->authDomain = authDomain; + authDomain = NULL; def->driverName = driverName; driverName = NULL; def->driverType = driverType; @@ -2408,6 +2424,8 @@ cleanup: VIR_FREE(hosts); VIR_FREE(protocol); VIR_FREE(device); + VIR_FREE(authDomain); + VIR_FREE(authId); VIR_FREE(driverType); VIR_FREE(driverName); VIR_FREE(cachetag); @@ -8645,12 +8663,19 @@ virDomainDiskDefFormat(virBufferPtr buf, if (def->src) { virBufferEscapeString(buf, " name='%s'", def->src); } - if (def->nhosts == 0) { + if (def->nhosts == 0 && def->authId == NULL) { virBufferAsprintf(buf, "/>\n"); } else { int i; virBufferAsprintf(buf, ">\n"); + if (def->authId) { + virBufferAsprintf(buf, " <auth id='%s'", + def->authId); + if (def->authDomain) + virBufferAsprintf(buf, " domain='%s'", def->authDomain); + virBufferStrcat(buf, "/>\n", NULL); + } for (i = 0; i < def->nhosts; i++) { virBufferEscapeString(buf, " <host name='%s'", def->hosts[i].name); diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index abf9cbd..8a997e1 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -221,6 +221,8 @@ struct _virDomainDiskDef { int protocol; int nhosts; virDomainDiskHostDefPtr hosts; + char *authDomain; /* ceph cluster name */ + char *authId; /* ceph auth id */ char *driverName; char *driverType; char *serial; -- 1.7.4.1
On Mon, Sep 19, 2011 at 09:13:40PM -0700, Sage Weil wrote:
Add additional fields to let you specify the how to authenticate with a network disk type. The authId is the name to authenticate as, and the authDomain optionally describes the domain that user exists in. The latter allows us to locate a secret in using the libvirt secrets API, as the user is may not unique if libvirt is talking to multiple backend clusters.
Signed-off-by: Sage Weil <sage@newdream.net> --- docs/schemas/domain.rng | 6 ++++++ src/conf/domain_conf.c | 43 ++++++++++++++++++++++++++++++++++--------- src/conf/domain_conf.h | 2 ++ 3 files changed, 42 insertions(+), 9 deletions(-)
diff --git a/docs/schemas/domain.rng b/docs/schemas/domain.rng index 6ccbeed..3574f03 100644 --- a/docs/schemas/domain.rng +++ b/docs/schemas/domain.rng @@ -736,6 +736,12 @@ </attribute> <optional> <attribute name="name"/> + <element name="auth"> + <attribute name="id"/> + <optional> + <attribute name="domain"/> + </optional> + </element> </optional> <zeroOrMore> <element name="host"> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 010ce57..5b80a9e 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2066,7 +2066,7 @@ virDomainDiskDefParseXML(virCapsPtr caps, unsigned int flags) { virDomainDiskDefPtr def; - xmlNodePtr cur, host; + xmlNodePtr cur, child; char *type = NULL; char *device = NULL; char *driverName = NULL; @@ -2084,6 +2084,8 @@ virDomainDiskDefParseXML(virCapsPtr caps, char *devaddr = NULL; virStorageEncryptionPtr encryption = NULL; char *serial = NULL; + char *authId = NULL; + char *authDomain = NULL;
if (VIR_ALLOC(def) < 0) { virReportOOMError(); @@ -2137,10 +2139,10 @@ virDomainDiskDefParseXML(virCapsPtr caps, _("missing name for disk source")); goto error; } - host = cur->children; - while (host != NULL) { - if (host->type == XML_ELEMENT_NODE && - xmlStrEqual(host->name, BAD_CAST "host")) { + child = cur->children; + while (child != NULL) { + if (child->type == XML_ELEMENT_NODE && + xmlStrEqual(child->name, BAD_CAST "host")) { if (VIR_REALLOC_N(hosts, nhosts + 1) < 0) { virReportOOMError(); goto error; @@ -2149,20 +2151,30 @@ virDomainDiskDefParseXML(virCapsPtr caps, hosts[nhosts].port = NULL; nhosts++;
- hosts[nhosts - 1].name = virXMLPropString(host, "name"); + hosts[nhosts - 1].name = virXMLPropString(child, "name"); if (!hosts[nhosts - 1].name) { virDomainReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("missing name for host")); goto error; } - hosts[nhosts - 1].port = virXMLPropString(host, "port"); + hosts[nhosts - 1].port = virXMLPropString(child, "port"); if (!hosts[nhosts - 1].port) { virDomainReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("missing port for host")); goto error; } } - host = host->next; + if (child->type == XML_ELEMENT_NODE && + xmlStrEqual(child->name, BAD_CAST "auth")) { + authId = virXMLPropString(child, "id"); + if (!authId) { + virDomainReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("missing id for auth")); + goto error; + } + authDomain = virXMLPropString(child, "domain"); + } + child = child->next; } break; default: @@ -2373,6 +2385,10 @@ virDomainDiskDefParseXML(virCapsPtr caps, hosts = NULL; def->nhosts = nhosts; nhosts = 0; + def->authId = authId; + authId = NULL; + def->authDomain = authDomain; + authDomain = NULL; def->driverName = driverName; driverName = NULL; def->driverType = driverType; @@ -2408,6 +2424,8 @@ cleanup: VIR_FREE(hosts); VIR_FREE(protocol); VIR_FREE(device); + VIR_FREE(authDomain); + VIR_FREE(authId); VIR_FREE(driverType); VIR_FREE(driverName); VIR_FREE(cachetag); @@ -8645,12 +8663,19 @@ virDomainDiskDefFormat(virBufferPtr buf, if (def->src) { virBufferEscapeString(buf, " name='%s'", def->src); } - if (def->nhosts == 0) { + if (def->nhosts == 0 && def->authId == NULL) { virBufferAsprintf(buf, "/>\n"); } else { int i;
virBufferAsprintf(buf, ">\n"); + if (def->authId) { + virBufferAsprintf(buf, " <auth id='%s'", + def->authId); + if (def->authDomain) + virBufferAsprintf(buf, " domain='%s'", def->authDomain); + virBufferStrcat(buf, "/>\n", NULL); + } for (i = 0; i < def->nhosts; i++) { virBufferEscapeString(buf, " <host name='%s'", def->hosts[i].name); diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index abf9cbd..8a997e1 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -221,6 +221,8 @@ struct _virDomainDiskDef { int protocol; int nhosts; virDomainDiskHostDefPtr hosts; + char *authDomain; /* ceph cluster name */ + char *authId; /* ceph auth id */ char *driverName; char *driverType; char *serial;
Based on my comments in patch 0, I would suggest something along the lines of: enum virDomainDiskSecretType { VIR_DOMAIN_DISK_SECRET_TYPE_NONE, VIR_DOMAIN_DISK_SECRET_TYPE_UUID, VIR_DOMAIN_DISK_SECRET_TYPE_USAGE, }; struct { char *username; int secretType; union { unsigned char uuid[VIR_UUID_BUFLEN]; char *usage; } secret; } auth; 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 :|
The qemu RBD driver needs access to the conn in order to get the secret needed for connecting to the ceph cluster. Signed-off-by: Sage Weil <sage@newdream.net> --- src/qemu/qemu_command.c | 5 +++-- src/qemu/qemu_command.h | 3 ++- src/qemu/qemu_driver.c | 17 ++++++++--------- src/qemu/qemu_hotplug.c | 15 +++++++++------ src/qemu/qemu_hotplug.h | 9 ++++++--- 5 files changed, 28 insertions(+), 21 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 76df0aa..32dcb79 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -1336,7 +1336,8 @@ qemuSafeSerialParamValue(const char *value) char * -qemuBuildDriveStr(virDomainDiskDefPtr disk, +qemuBuildDriveStr(virConnectPtr conn, + virDomainDiskDefPtr disk, bool bootable, virBitmapPtr qemuCaps) { @@ -3460,7 +3461,7 @@ qemuBuildCommandLine(virConnectPtr conn, deviceFlagMasked = true; } } - optstr = qemuBuildDriveStr(disk, + optstr = qemuBuildDriveStr(conn, disk, emitBootindex ? false : !!bootindex, qemuCaps); if (deviceFlagMasked) diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h index 87660f2..4a90544 100644 --- a/src/qemu/qemu_command.h +++ b/src/qemu/qemu_command.h @@ -75,7 +75,8 @@ char *qemuDeviceDriveHostAlias(virDomainDiskDefPtr disk, virBitmapPtr qemuCaps); /* Both legacy & current support */ -char *qemuBuildDriveStr(virDomainDiskDefPtr disk, +char *qemuBuildDriveStr(virConnectPtr conn, + virDomainDiskDefPtr disk, bool bootable, virBitmapPtr qemuCaps); char *qemuBuildFSStr(virDomainFSDefPtr fs, diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index ce19be7..a1b73e7 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -4773,7 +4773,8 @@ qemudDomainUndefine(virDomainPtr dom) } static int -qemuDomainAttachDeviceDiskLive(struct qemud_driver *driver, +qemuDomainAttachDeviceDiskLive(virConnectPtr conn, + struct qemud_driver *driver, virDomainObjPtr vm, virDomainDeviceDefPtr dev) { @@ -4805,12 +4806,12 @@ qemuDomainAttachDeviceDiskLive(struct qemud_driver *driver, break; case VIR_DOMAIN_DISK_DEVICE_DISK: if (disk->bus == VIR_DOMAIN_DISK_BUS_USB) - ret = qemuDomainAttachUsbMassstorageDevice(driver, vm, + ret = qemuDomainAttachUsbMassstorageDevice(conn, driver, vm, disk); else if (disk->bus == VIR_DOMAIN_DISK_BUS_VIRTIO) - ret = qemuDomainAttachPciDiskDevice(driver, vm, disk); + ret = qemuDomainAttachPciDiskDevice(conn, driver, vm, disk); else if (disk->bus == VIR_DOMAIN_DISK_BUS_SCSI) - ret = qemuDomainAttachSCSIDisk(driver, vm, disk); + ret = qemuDomainAttachSCSIDisk(conn, driver, vm, disk); else qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("disk bus '%s' cannot be hotplugged."), @@ -4866,7 +4867,7 @@ qemuDomainAttachDeviceLive(virDomainObjPtr vm, switch (dev->type) { case VIR_DOMAIN_DEVICE_DISK: qemuDomainObjCheckDiskTaint(driver, vm, dev->data.disk, -1); - ret = qemuDomainAttachDeviceDiskLive(driver, vm, dev); + ret = qemuDomainAttachDeviceDiskLive(dom->conn, driver, vm, dev); if (!ret) dev->data.disk = NULL; break; @@ -5396,8 +5397,7 @@ static int qemuDomainAttachDeviceFlags(virDomainPtr dom, const char *xml, static int qemuDomainAttachDevice(virDomainPtr dom, const char *xml) { - return qemuDomainAttachDeviceFlags(dom, xml, - VIR_DOMAIN_AFFECT_LIVE); + return qemuDomainAttachDeviceFlags(dom, xml, VIR_DOMAIN_AFFECT_LIVE); } @@ -5416,8 +5416,7 @@ static int qemuDomainDetachDeviceFlags(virDomainPtr dom, const char *xml, static int qemuDomainDetachDevice(virDomainPtr dom, const char *xml) { - return qemuDomainDetachDeviceFlags(dom, xml, - VIR_DOMAIN_AFFECT_LIVE); + return qemuDomainDetachDeviceFlags(dom, xml, VIR_DOMAIN_AFFECT_LIVE); } static int qemudDomainGetAutostart(virDomainPtr dom, diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 5f449fb..3568609 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -151,7 +151,8 @@ error: } -int qemuDomainAttachPciDiskDevice(struct qemud_driver *driver, +int qemuDomainAttachPciDiskDevice(virConnectPtr conn, + struct qemud_driver *driver, virDomainObjPtr vm, virDomainDiskDefPtr disk) { @@ -187,7 +188,7 @@ int qemuDomainAttachPciDiskDevice(struct qemud_driver *driver, if (qemuAssignDeviceDiskAlias(disk, priv->qemuCaps) < 0) goto error; - if (!(drivestr = qemuBuildDriveStr(disk, false, priv->qemuCaps))) + if (!(drivestr = qemuBuildDriveStr(conn, disk, false, priv->qemuCaps))) goto error; if (!(devstr = qemuBuildDriveDevStr(disk, 0, priv->qemuCaps))) @@ -372,7 +373,8 @@ qemuDomainFindOrCreateSCSIDiskController(struct qemud_driver *driver, } -int qemuDomainAttachSCSIDisk(struct qemud_driver *driver, +int qemuDomainAttachSCSIDisk(virConnectPtr conn, + struct qemud_driver *driver, virDomainObjPtr vm, virDomainDiskDefPtr disk) { @@ -416,7 +418,7 @@ int qemuDomainAttachSCSIDisk(struct qemud_driver *driver, goto error; } - if (!(drivestr = qemuBuildDriveStr(disk, false, priv->qemuCaps))) + if (!(drivestr = qemuBuildDriveStr(conn, disk, false, priv->qemuCaps))) goto error; for (i = 0 ; i <= disk->info.addr.drive.controller ; i++) { @@ -495,7 +497,8 @@ error: } -int qemuDomainAttachUsbMassstorageDevice(struct qemud_driver *driver, +int qemuDomainAttachUsbMassstorageDevice(virConnectPtr conn, + struct qemud_driver *driver, virDomainObjPtr vm, virDomainDiskDefPtr disk) { @@ -532,7 +535,7 @@ int qemuDomainAttachUsbMassstorageDevice(struct qemud_driver *driver, if (qemuCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE)) { if (qemuAssignDeviceDiskAlias(disk, priv->qemuCaps) < 0) goto error; - if (!(drivestr = qemuBuildDriveStr(disk, false, priv->qemuCaps))) + if (!(drivestr = qemuBuildDriveStr(conn, disk, false, priv->qemuCaps))) goto error; if (!(devstr = qemuBuildDriveDevStr(disk, 0, priv->qemuCaps))) goto error; diff --git a/src/qemu/qemu_hotplug.h b/src/qemu/qemu_hotplug.h index 009f1f6..14017bf 100644 --- a/src/qemu/qemu_hotplug.h +++ b/src/qemu/qemu_hotplug.h @@ -31,16 +31,19 @@ int qemuDomainChangeEjectableMedia(struct qemud_driver *driver, virDomainObjPtr vm, virDomainDiskDefPtr disk, bool force); -int qemuDomainAttachPciDiskDevice(struct qemud_driver *driver, +int qemuDomainAttachPciDiskDevice(virConnectPtr conn, + struct qemud_driver *driver, virDomainObjPtr vm, virDomainDiskDefPtr disk); int qemuDomainAttachPciControllerDevice(struct qemud_driver *driver, virDomainObjPtr vm, virDomainControllerDefPtr controller); -int qemuDomainAttachSCSIDisk(struct qemud_driver *driver, +int qemuDomainAttachSCSIDisk(virConnectPtr conn, + struct qemud_driver *driver, virDomainObjPtr vm, virDomainDiskDefPtr disk); -int qemuDomainAttachUsbMassstorageDevice(struct qemud_driver *driver, +int qemuDomainAttachUsbMassstorageDevice(virConnectPtr conn, + struct qemud_driver *driver, virDomainObjPtr vm, virDomainDiskDefPtr disk); int qemuDomainAttachNetDevice(virConnectPtr conn, -- 1.7.4.1
On Mon, Sep 19, 2011 at 09:13:41PM -0700, Sage Weil wrote:
The qemu RBD driver needs access to the conn in order to get the secret needed for connecting to the ceph cluster.
Signed-off-by: Sage Weil <sage@newdream.net> --- src/qemu/qemu_command.c | 5 +++-- src/qemu/qemu_command.h | 3 ++- src/qemu/qemu_driver.c | 17 ++++++++--------- src/qemu/qemu_hotplug.c | 15 +++++++++------ src/qemu/qemu_hotplug.h | 9 ++++++--- 5 files changed, 28 insertions(+), 21 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 76df0aa..32dcb79 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -1336,7 +1336,8 @@ qemuSafeSerialParamValue(const char *value)
char * -qemuBuildDriveStr(virDomainDiskDefPtr disk, +qemuBuildDriveStr(virConnectPtr conn, + virDomainDiskDefPtr disk, bool bootable, virBitmapPtr qemuCaps) { @@ -3460,7 +3461,7 @@ qemuBuildCommandLine(virConnectPtr conn, deviceFlagMasked = true; } } - optstr = qemuBuildDriveStr(disk, + optstr = qemuBuildDriveStr(conn, disk, emitBootindex ? false : !!bootindex, qemuCaps); if (deviceFlagMasked) diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h index 87660f2..4a90544 100644 --- a/src/qemu/qemu_command.h +++ b/src/qemu/qemu_command.h @@ -75,7 +75,8 @@ char *qemuDeviceDriveHostAlias(virDomainDiskDefPtr disk, virBitmapPtr qemuCaps);
/* Both legacy & current support */ -char *qemuBuildDriveStr(virDomainDiskDefPtr disk, +char *qemuBuildDriveStr(virConnectPtr conn, + virDomainDiskDefPtr disk, bool bootable, virBitmapPtr qemuCaps); char *qemuBuildFSStr(virDomainFSDefPtr fs, diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index ce19be7..a1b73e7 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -4773,7 +4773,8 @@ qemudDomainUndefine(virDomainPtr dom) }
static int -qemuDomainAttachDeviceDiskLive(struct qemud_driver *driver, +qemuDomainAttachDeviceDiskLive(virConnectPtr conn, + struct qemud_driver *driver, virDomainObjPtr vm, virDomainDeviceDefPtr dev) { @@ -4805,12 +4806,12 @@ qemuDomainAttachDeviceDiskLive(struct qemud_driver *driver, break; case VIR_DOMAIN_DISK_DEVICE_DISK: if (disk->bus == VIR_DOMAIN_DISK_BUS_USB) - ret = qemuDomainAttachUsbMassstorageDevice(driver, vm, + ret = qemuDomainAttachUsbMassstorageDevice(conn, driver, vm, disk); else if (disk->bus == VIR_DOMAIN_DISK_BUS_VIRTIO) - ret = qemuDomainAttachPciDiskDevice(driver, vm, disk); + ret = qemuDomainAttachPciDiskDevice(conn, driver, vm, disk); else if (disk->bus == VIR_DOMAIN_DISK_BUS_SCSI) - ret = qemuDomainAttachSCSIDisk(driver, vm, disk); + ret = qemuDomainAttachSCSIDisk(conn, driver, vm, disk); else qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("disk bus '%s' cannot be hotplugged."), @@ -4866,7 +4867,7 @@ qemuDomainAttachDeviceLive(virDomainObjPtr vm, switch (dev->type) { case VIR_DOMAIN_DEVICE_DISK: qemuDomainObjCheckDiskTaint(driver, vm, dev->data.disk, -1); - ret = qemuDomainAttachDeviceDiskLive(driver, vm, dev); + ret = qemuDomainAttachDeviceDiskLive(dom->conn, driver, vm, dev); if (!ret) dev->data.disk = NULL; break; @@ -5396,8 +5397,7 @@ static int qemuDomainAttachDeviceFlags(virDomainPtr dom, const char *xml,
static int qemuDomainAttachDevice(virDomainPtr dom, const char *xml) { - return qemuDomainAttachDeviceFlags(dom, xml, - VIR_DOMAIN_AFFECT_LIVE); + return qemuDomainAttachDeviceFlags(dom, xml, VIR_DOMAIN_AFFECT_LIVE); }
@@ -5416,8 +5416,7 @@ static int qemuDomainDetachDeviceFlags(virDomainPtr dom, const char *xml,
static int qemuDomainDetachDevice(virDomainPtr dom, const char *xml) { - return qemuDomainDetachDeviceFlags(dom, xml, - VIR_DOMAIN_AFFECT_LIVE); + return qemuDomainDetachDeviceFlags(dom, xml, VIR_DOMAIN_AFFECT_LIVE); }
static int qemudDomainGetAutostart(virDomainPtr dom, diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 5f449fb..3568609 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -151,7 +151,8 @@ error: }
-int qemuDomainAttachPciDiskDevice(struct qemud_driver *driver, +int qemuDomainAttachPciDiskDevice(virConnectPtr conn, + struct qemud_driver *driver, virDomainObjPtr vm, virDomainDiskDefPtr disk) { @@ -187,7 +188,7 @@ int qemuDomainAttachPciDiskDevice(struct qemud_driver *driver, if (qemuAssignDeviceDiskAlias(disk, priv->qemuCaps) < 0) goto error;
- if (!(drivestr = qemuBuildDriveStr(disk, false, priv->qemuCaps))) + if (!(drivestr = qemuBuildDriveStr(conn, disk, false, priv->qemuCaps))) goto error;
if (!(devstr = qemuBuildDriveDevStr(disk, 0, priv->qemuCaps))) @@ -372,7 +373,8 @@ qemuDomainFindOrCreateSCSIDiskController(struct qemud_driver *driver, }
-int qemuDomainAttachSCSIDisk(struct qemud_driver *driver, +int qemuDomainAttachSCSIDisk(virConnectPtr conn, + struct qemud_driver *driver, virDomainObjPtr vm, virDomainDiskDefPtr disk) { @@ -416,7 +418,7 @@ int qemuDomainAttachSCSIDisk(struct qemud_driver *driver, goto error; }
- if (!(drivestr = qemuBuildDriveStr(disk, false, priv->qemuCaps))) + if (!(drivestr = qemuBuildDriveStr(conn, disk, false, priv->qemuCaps))) goto error;
for (i = 0 ; i <= disk->info.addr.drive.controller ; i++) { @@ -495,7 +497,8 @@ error: }
-int qemuDomainAttachUsbMassstorageDevice(struct qemud_driver *driver, +int qemuDomainAttachUsbMassstorageDevice(virConnectPtr conn, + struct qemud_driver *driver, virDomainObjPtr vm, virDomainDiskDefPtr disk) { @@ -532,7 +535,7 @@ int qemuDomainAttachUsbMassstorageDevice(struct qemud_driver *driver, if (qemuCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE)) { if (qemuAssignDeviceDiskAlias(disk, priv->qemuCaps) < 0) goto error; - if (!(drivestr = qemuBuildDriveStr(disk, false, priv->qemuCaps))) + if (!(drivestr = qemuBuildDriveStr(conn, disk, false, priv->qemuCaps))) goto error; if (!(devstr = qemuBuildDriveDevStr(disk, 0, priv->qemuCaps))) goto error; diff --git a/src/qemu/qemu_hotplug.h b/src/qemu/qemu_hotplug.h index 009f1f6..14017bf 100644 --- a/src/qemu/qemu_hotplug.h +++ b/src/qemu/qemu_hotplug.h @@ -31,16 +31,19 @@ int qemuDomainChangeEjectableMedia(struct qemud_driver *driver, virDomainObjPtr vm, virDomainDiskDefPtr disk, bool force); -int qemuDomainAttachPciDiskDevice(struct qemud_driver *driver, +int qemuDomainAttachPciDiskDevice(virConnectPtr conn, + struct qemud_driver *driver, virDomainObjPtr vm, virDomainDiskDefPtr disk); int qemuDomainAttachPciControllerDevice(struct qemud_driver *driver, virDomainObjPtr vm, virDomainControllerDefPtr controller); -int qemuDomainAttachSCSIDisk(struct qemud_driver *driver, +int qemuDomainAttachSCSIDisk(virConnectPtr conn, + struct qemud_driver *driver, virDomainObjPtr vm, virDomainDiskDefPtr disk); -int qemuDomainAttachUsbMassstorageDevice(struct qemud_driver *driver, +int qemuDomainAttachUsbMassstorageDevice(virConnectPtr conn, + struct qemud_driver *driver, virDomainObjPtr vm, virDomainDiskDefPtr disk); int qemuDomainAttachNetDevice(virConnectPtr conn,
ACK, this all looks fine. In fact given that it wasn't there already, I have a feeling that hotplug of qcow2 encrypted disks is currently broken, which is something I need to look into. 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 :|
Implement a generic helper to escape a given set of characters with a leading '\'. Generalizes virBufferEscapeSexpr(). Signed-off-by: Sage Weil <sage@newdream.net> --- src/libvirt_private.syms | 1 + src/util/buf.c | 33 ++++++++++++++++++++++++++------- src/util/buf.h | 1 + 3 files changed, 28 insertions(+), 7 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 830222b..d230fab 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -25,6 +25,7 @@ virBufferAddChar; virBufferAsprintf; virBufferContentAndReset; virBufferError; +virBufferEscape; virBufferEscapeSexpr; virBufferEscapeString; virBufferFreeAndReset; diff --git a/src/util/buf.c b/src/util/buf.c index 5002486..7d0d2d3 100644 --- a/src/util/buf.c +++ b/src/util/buf.c @@ -383,9 +383,29 @@ virBufferEscapeSexpr(const virBufferPtr buf, const char *format, const char *str) { + virBufferEscape(buf, "\\\'", format, str); +} + +/** + * virBufferEscape: + * @buf: the buffer to dump + * @toescape: NULL-terminated list of characters to escape + * @format: a printf like format string but with only one %s parameter + * @str: the string argument which need to be escaped + * + * Do a formatted print with a single string to a buffer. Any characters + * in the provided list are escaped with a preceeding \. + */ +void +virBufferEscape(const virBufferPtr buf, + const char *toescape, + const char *format, + const char *str) +{ int len; char *escaped, *out; const char *cur; + const char *p; if ((format == NULL) || (buf == NULL) || (str == NULL)) return; @@ -408,14 +428,13 @@ virBufferEscapeSexpr(const virBufferPtr buf, cur = str; out = escaped; while (*cur != 0) { - switch (*cur) { - case '\\': - case '\'': - *out++ = '\\'; - /* fallthrough */ - default: - *out++ = *cur; + for (p = toescape; *p; ++p) { + if (*cur == *p) { + *out++ = '\\'; + break; + } } + *out++ = *cur; cur++; } *out = 0; diff --git a/src/util/buf.h b/src/util/buf.h index 06d01ba..e545ed9 100644 --- a/src/util/buf.h +++ b/src/util/buf.h @@ -50,6 +50,7 @@ void virBufferStrcat(const virBufferPtr buf, ...) ATTRIBUTE_SENTINEL; void virBufferEscapeString(const virBufferPtr buf, const char *format, const char *str); void virBufferEscapeSexpr(const virBufferPtr buf, const char *format, const char *str); +void virBufferEscape(const virBufferPtr buf, const char *toescape, const char *format, const char *str); void virBufferURIEncodeString (const virBufferPtr buf, const char *str); # define virBufferAddLit(buf_, literal_string_) \ -- 1.7.4.1
On Mon, Sep 19, 2011 at 09:13:42PM -0700, Sage Weil wrote:
Implement a generic helper to escape a given set of characters with a leading '\'. Generalizes virBufferEscapeSexpr().
Signed-off-by: Sage Weil <sage@newdream.net> --- src/libvirt_private.syms | 1 + src/util/buf.c | 33 ++++++++++++++++++++++++++------- src/util/buf.h | 1 + 3 files changed, 28 insertions(+), 7 deletions(-)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 830222b..d230fab 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -25,6 +25,7 @@ virBufferAddChar; virBufferAsprintf; virBufferContentAndReset; virBufferError; +virBufferEscape; virBufferEscapeSexpr; virBufferEscapeString; virBufferFreeAndReset; diff --git a/src/util/buf.c b/src/util/buf.c index 5002486..7d0d2d3 100644 --- a/src/util/buf.c +++ b/src/util/buf.c @@ -383,9 +383,29 @@ virBufferEscapeSexpr(const virBufferPtr buf, const char *format, const char *str) { + virBufferEscape(buf, "\\\'", format, str); +} + +/** + * virBufferEscape: + * @buf: the buffer to dump + * @toescape: NULL-terminated list of characters to escape + * @format: a printf like format string but with only one %s parameter + * @str: the string argument which need to be escaped + * + * Do a formatted print with a single string to a buffer. Any characters + * in the provided list are escaped with a preceeding \. + */ +void +virBufferEscape(const virBufferPtr buf, + const char *toescape, + const char *format, + const char *str) +{ int len; char *escaped, *out; const char *cur; + const char *p;
if ((format == NULL) || (buf == NULL) || (str == NULL)) return; @@ -408,14 +428,13 @@ virBufferEscapeSexpr(const virBufferPtr buf, cur = str; out = escaped; while (*cur != 0) { - switch (*cur) { - case '\\': - case '\'': - *out++ = '\\'; - /* fallthrough */ - default: - *out++ = *cur; + for (p = toescape; *p; ++p) { + if (*cur == *p) { + *out++ = '\\'; + break; + } } + *out++ = *cur; cur++; } *out = 0; diff --git a/src/util/buf.h b/src/util/buf.h index 06d01ba..e545ed9 100644 --- a/src/util/buf.h +++ b/src/util/buf.h @@ -50,6 +50,7 @@ void virBufferStrcat(const virBufferPtr buf, ...) ATTRIBUTE_SENTINEL; void virBufferEscapeString(const virBufferPtr buf, const char *format, const char *str); void virBufferEscapeSexpr(const virBufferPtr buf, const char *format, const char *str); +void virBufferEscape(const virBufferPtr buf, const char *toescape, const char *format, const char *str); void virBufferURIEncodeString (const virBufferPtr buf, const char *str);
# define virBufferAddLit(buf_, literal_string_) \
ACK, trivial isolated patch 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 :|
On 10/11/2011 04:39 AM, Daniel P. Berrange wrote:
On Mon, Sep 19, 2011 at 09:13:42PM -0700, Sage Weil wrote:
Implement a generic helper to escape a given set of characters with a leading '\'. Generalizes virBufferEscapeSexpr().
Signed-off-by: Sage Weil<sage@newdream.net> @@ -408,14 +428,13 @@ virBufferEscapeSexpr(const virBufferPtr buf, cur = str; out = escaped; while (*cur != 0) { - switch (*cur) { - case '\\': - case '\'': - *out++ = '\\'; - /* fallthrough */ - default: - *out++ = *cur; + for (p = toescape; *p; ++p) { + if (*cur == *p) {
strchr is slightly more efficient than hand-rolling this loop. More importantly, you had a logic bug, where you replaced one hard-coded instance of "\\'", but not the other (the strcspn optimization). Also, I tend to use "'" instead of "\'", although both work, since I favor concise code (I save my ramblings for my emails :).
ACK, trivial isolated patch
I added you to AUTHORS, squashed this in, then pushed. diff --git i/src/util/buf.c w/src/util/buf.c index 7d0d2d3..5627d8f 100644 --- i/src/util/buf.c +++ w/src/util/buf.c @@ -383,7 +383,7 @@ virBufferEscapeSexpr(const virBufferPtr buf, const char *format, const char *str) { - virBufferEscape(buf, "\\\'", format, str); + virBufferEscape(buf, "\\'", format, str); } /** @@ -414,7 +414,7 @@ virBufferEscape(const virBufferPtr buf, return; len = strlen(str); - if (strcspn(str, "\\'") == len) { + if (strcspn(str, toescape) == len) { virBufferAsprintf(buf, format, str); return; } @@ -428,12 +428,8 @@ virBufferEscape(const virBufferPtr buf, cur = str; out = escaped; while (*cur != 0) { - for (p = toescape; *p; ++p) { - if (*cur == *p) { - *out++ = '\\'; - break; - } - } + if (strchr(toescape, *cur)) + *out++ = '\\'; *out++ = *cur; cur++; } -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
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 id property is the Ceph/RBD user to authenticate as, and the domain property is a identifier (local to libvirt) for the Ceph cluster in question. If both are specified, we look for a libvirt secret of type CEPH with matching id and domain properties. 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 unit tests accordingly. Signed-off-by: Sage Weil <sage@newdream.net> --- src/qemu/qemu_command.c | 268 +++++++++++--------- .../qemuxml2argv-disk-drive-network-rbd.args | 6 +- .../qemuxml2argv-disk-drive-network-rbd.xml | 1 + 3 files changed, 157 insertions(+), 118 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 32dcb79..00fb169 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> @@ -1334,6 +1335,146 @@ qemuSafeSerialParamValue(const char *value) return 0; } +static int buildRBDString(virConnectPtr conn, + virDomainDiskDefPtr disk, + virBufferPtr opt) +{ + int i; + char idDomain[80]; + virSecretPtr sec; + char *secret; + size_t secret_size; + + virBufferAsprintf(opt, "rbd:%s", disk->src); + if (disk->authId) { + virBufferEscape(opt, ":", ":id=%s", disk->authId); + } + if (disk->authDomain) { + /* look up secret */ + snprintf(idDomain, sizeof(idDomain), "%s/%s", disk->authId, + disk->authDomain); + sec = virSecretLookupByUsage(conn, + VIR_SECRET_USAGE_TYPE_CEPH, + idDomain); + if (sec) { + char *base64; + + secret = (char *)conn->secretDriver->getValue(sec, &secret_size, 0, + VIR_SECRET_GET_VALUE_INTERNAL_CALL); + /* 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 { + VIR_WARN("rbd id '%s' domain '%s' specified but secret not found", + disk->authId, disk->authDomain); + } + } + 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) +{ + 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) +{ + 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->authId = 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, @@ -1453,8 +1594,9 @@ 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); + buildRBDString(conn, disk, &opt); + virBufferStrcat(&opt, ",", NULL); break; case VIR_DOMAIN_DISK_PROTOCOL_SHEEPDOG: if (disk->nhosts == 0) @@ -2851,8 +2993,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; uname_normalize(&ut); @@ -3413,7 +3553,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 */ @@ -3471,26 +3610,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) @@ -3528,7 +3647,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) { @@ -3597,24 +3715,11 @@ 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; + buildRBDString(conn, disk, &opt); + virAsprintf(&file, "%s", + virBufferContentAndReset(&opt)); } break; case VIR_DOMAIN_DISK_PROTOCOL_SHEEPDOG: @@ -3643,9 +3748,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; @@ -4777,7 +4879,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; } @@ -4809,10 +4910,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++; @@ -5091,6 +5188,8 @@ qemuParseCommandLineDisk(virCapsPtr caps, virReportOOMError(); goto cleanup; } + if (parseRBDString(def) < 0) + goto cleanup; VIR_FREE(p); } else if (STRPREFIX(def->src, "sheepdog:")) { @@ -6195,7 +6294,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] */ @@ -6534,68 +6634,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.args b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd.args index 3ab1219..68b9e95 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:id=myname: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.xml b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd.xml index e920db1..f090834 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd.xml +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd.xml @@ -22,6 +22,7 @@ <disk type='network' device='disk'> <driver name='qemu' type='raw'/> <source protocol='rbd' name='pool/image'> + <auth id='myname'/> <host name='mon1.example.org' port='6321'/> <host name='mon2.example.org' port='6322'/> <host name='mon3.example.org' port='6322'/> -- 1.7.4.1
On Mon, Sep 19, 2011 at 09:13:43PM -0700, Sage Weil wrote:
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 id property is the Ceph/RBD user to authenticate as, and the domain property is a identifier (local to libvirt) for the Ceph cluster in question. If both are specified, we look for a libvirt secret of type CEPH with matching id and domain properties.
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 unit tests accordingly.
Signed-off-by: Sage Weil <sage@newdream.net> --- src/qemu/qemu_command.c | 268 +++++++++++--------- .../qemuxml2argv-disk-drive-network-rbd.args | 6 +- .../qemuxml2argv-disk-drive-network-rbd.xml | 1 + 3 files changed, 157 insertions(+), 118 deletions(-) +static int buildRBDString(virConnectPtr conn, + virDomainDiskDefPtr disk, + virBufferPtr opt) +{ + int i; + char idDomain[80]; + virSecretPtr sec; + char *secret; + size_t secret_size; + + virBufferAsprintf(opt, "rbd:%s", disk->src); + if (disk->authId) { + virBufferEscape(opt, ":", ":id=%s", disk->authId); + } + if (disk->authDomain) { + /* look up secret */ + snprintf(idDomain, sizeof(idDomain), "%s/%s", disk->authId, + disk->authDomain); + sec = virSecretLookupByUsage(conn, + VIR_SECRET_USAGE_TYPE_CEPH, + idDomain);
This is where you'd want to use virSecretLookupByUUID.
+ if (sec) { + char *base64; + + secret = (char *)conn->secretDriver->getValue(sec, &secret_size, 0, + VIR_SECRET_GET_VALUE_INTERNAL_CALL);
No need for the cast to 'char *', since void * casts to anything in C. But you do need to handle case of the return'd secret being 'NULL' and return to the caller in that case.
+ /* 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 { + VIR_WARN("rbd id '%s' domain '%s' specified but secret not found", + disk->authId, disk->authDomain);
You ought to use qemuReportError() here and return to the caller
+ } + } + 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; +} +
@@ -1453,8 +1594,9 @@ 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); + buildRBDString(conn, disk, &opt); + virBufferStrcat(&opt, ",", NULL); break; case VIR_DOMAIN_DISK_PROTOCOL_SHEEPDOG: if (disk->nhosts == 0) @@ -3597,24 +3715,11 @@ 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; + buildRBDString(conn, disk, &opt); + virAsprintf(&file, "%s", + virBufferContentAndReset(&opt));
This last statement leaks. 'virBufferContentAndReset' gives you back the internal buffer to keep, so there's no need to duplicate it using asprintf. Also you need to check for an error. So you want this if (virBufferError(&opt)) { virReportOOMError(); goto cleanup; } file = virBufferContentAndReset(&opt); Generally I think you're going in the right direction with this patch. 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 :|
Hi everyone, Any feedback on this series? I touch a few pieces outside of the RBD bits here... Thanks! sage On Mon, 19 Sep 2011, Sage Weil wrote:
The current support for qemu and Ceph RBD (rados block device) has two main deficiencies: authentication doesn't work, and it relies on environment variables (which don't work with latest upstream). This patch set addresses both those problems.
The first two patches update the xml schemas and conf to add a Ceph secret type and to specify authentication information along with the rbd disk.
The next two patches make some libvirt changes. We pass virConnectPtr down into the Domain{Attach,Detach} methods (needed to access secrets while building the qemu command), and add a helper that will escape arbitrary characters.
The final patch replaces the current RBD qemu code and uses the new conf info to do authentication properly. (We still need to make a change there to avoid having the authentication key show up on qemu command line; I'll clean that up shortly.)
Comments on this approach?
Thanks! sage
Changes from v1: update docs/schemas/{domain,secret}.rng
Sage Weil (5): secret: add Ceph secret type storage: add authId, authDomain to virDomainDiskDef qemu: pass virConnectPtr into Domain{Attach,Detach}* buf: implement generic virBufferEscape qemu/rbd: improve rbd device specification
docs/schemas/domain.rng | 6 + docs/schemas/secret.rng | 17 ++ include/libvirt/libvirt.h.in | 3 + src/conf/domain_conf.c | 43 +++- src/conf/domain_conf.h | 2 + src/conf/secret_conf.c | 45 ++++- src/conf/secret_conf.h | 1 + src/libvirt_private.syms | 1 + src/qemu/qemu_command.c | 273 +++++++++++--------- src/qemu/qemu_command.h | 3 +- src/qemu/qemu_driver.c | 17 +- src/qemu/qemu_hotplug.c | 15 +- src/qemu/qemu_hotplug.h | 9 +- src/secret/secret_driver.c | 8 + src/util/buf.c | 33 ++- src/util/buf.h | 1 + .../qemuxml2argv-disk-drive-network-rbd.args | 6 +- .../qemuxml2argv-disk-drive-network-rbd.xml | 1 + 18 files changed, 328 insertions(+), 156 deletions(-)
-- 1.7.4.1
From 498cd06b76bbb4415a2f81f9d169f267ff99329c Mon Sep 17 00:00:00 2001 From: Sage Weil <sage@newdream.net> Date: Thu, 15 Sep 2011 13:47:40 -0700 Subject: [PATCH 0/5] Improve Ceph Qemu+RBD support
The current support for qemu and Ceph RBD (rados block device) has two main deficiencies: authentication doesn't work, and it relies on environment variables (which don't work with latest upstream).
This patch set addresses both those problems, while trying to integrate as cleanly as possible with the rest of libvirt.
The first few patches make some changes to libvirt itself: adding a CEPH secret type (for Ceph/RBD authentication), adding authentication fields to the XML schema, passing the virConnectPtr into the Domain{Attach,Detach} methods (needed to access secrets while building the qemu command), a helper that will escape arbitrary characters, and finally a patch that replaces the current RBD qemu code.
Comments on this approach?
Thanks! sage
Sage Weil (5): secret: add Ceph secret type storage: add authId, authDomain to virDomainDiskDef qemu: pass virConnectPtr into Domain{Attach,Detach}* buf: implement generic virBufferEscape qemu/rbd: improve rbd device specification
include/libvirt/libvirt.h.in | 3 + src/conf/domain_conf.c | 43 +++- src/conf/domain_conf.h | 2 + src/conf/secret_conf.c | 45 ++++- src/conf/secret_conf.h | 1 + src/libvirt_private.syms | 1 + src/qemu/qemu_command.c | 273 +++++++++++--------- src/qemu/qemu_command.h | 3 +- src/qemu/qemu_driver.c | 17 +- src/qemu/qemu_hotplug.c | 15 +- src/qemu/qemu_hotplug.h | 9 +- src/secret/secret_driver.c | 8 + src/util/buf.c | 33 ++- src/util/buf.h | 1 + .../qemuxml2argv-disk-drive-network-rbd.args | 6 +- .../qemuxml2argv-disk-drive-network-rbd.xml | 1 + 16 files changed, 305 insertions(+), 156 deletions(-)
-- 1.7.4.1
-- To unsubscribe from this list: send the line "unsubscribe ceph-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On Mon, 2011-09-19 at 21:13 -0700, Sage Weil wrote:
The current support for qemu and Ceph RBD (rados block device) has two main deficiencies: authentication doesn't work, and it relies on environment variables (which don't work with latest upstream). This patch set addresses both those problems.
Authentication is something that is really needed in the libvirt implementation of RBD. The interest in RBD is growing rapidly, but without auth support it's kind of useless. Would like to see this implemented! Wido
The first two patches update the xml schemas and conf to add a Ceph secret type and to specify authentication information along with the rbd disk.
The next two patches make some libvirt changes. We pass virConnectPtr down into the Domain{Attach,Detach} methods (needed to access secrets while building the qemu command), and add a helper that will escape arbitrary characters.
The final patch replaces the current RBD qemu code and uses the new conf info to do authentication properly. (We still need to make a change there to avoid having the authentication key show up on qemu command line; I'll clean that up shortly.)
Comments on this approach?
Thanks! sage
Changes from v1: update docs/schemas/{domain,secret}.rng
Sage Weil (5): secret: add Ceph secret type storage: add authId, authDomain to virDomainDiskDef qemu: pass virConnectPtr into Domain{Attach,Detach}* buf: implement generic virBufferEscape qemu/rbd: improve rbd device specification
docs/schemas/domain.rng | 6 + docs/schemas/secret.rng | 17 ++ include/libvirt/libvirt.h.in | 3 + src/conf/domain_conf.c | 43 +++- src/conf/domain_conf.h | 2 + src/conf/secret_conf.c | 45 ++++- src/conf/secret_conf.h | 1 + src/libvirt_private.syms | 1 + src/qemu/qemu_command.c | 273 +++++++++++--------- src/qemu/qemu_command.h | 3 +- src/qemu/qemu_driver.c | 17 +- src/qemu/qemu_hotplug.c | 15 +- src/qemu/qemu_hotplug.h | 9 +- src/secret/secret_driver.c | 8 + src/util/buf.c | 33 ++- src/util/buf.h | 1 + .../qemuxml2argv-disk-drive-network-rbd.args | 6 +- .../qemuxml2argv-disk-drive-network-rbd.xml | 1 + 18 files changed, 328 insertions(+), 156 deletions(-)
-- 1.7.4.1
From 498cd06b76bbb4415a2f81f9d169f267ff99329c Mon Sep 17 00:00:00 2001 From: Sage Weil <sage@newdream.net> Date: Thu, 15 Sep 2011 13:47:40 -0700 Subject: [PATCH 0/5] Improve Ceph Qemu+RBD support
The current support for qemu and Ceph RBD (rados block device) has two main deficiencies: authentication doesn't work, and it relies on environment variables (which don't work with latest upstream).
This patch set addresses both those problems, while trying to integrate as cleanly as possible with the rest of libvirt.
The first few patches make some changes to libvirt itself: adding a CEPH secret type (for Ceph/RBD authentication), adding authentication fields to the XML schema, passing the virConnectPtr into the Domain{Attach,Detach} methods (needed to access secrets while building the qemu command), a helper that will escape arbitrary characters, and finally a patch that replaces the current RBD qemu code.
Comments on this approach?
Thanks! sage
Sage Weil (5): secret: add Ceph secret type storage: add authId, authDomain to virDomainDiskDef qemu: pass virConnectPtr into Domain{Attach,Detach}* buf: implement generic virBufferEscape qemu/rbd: improve rbd device specification
include/libvirt/libvirt.h.in | 3 + src/conf/domain_conf.c | 43 +++- src/conf/domain_conf.h | 2 + src/conf/secret_conf.c | 45 ++++- src/conf/secret_conf.h | 1 + src/libvirt_private.syms | 1 + src/qemu/qemu_command.c | 273 +++++++++++--------- src/qemu/qemu_command.h | 3 +- src/qemu/qemu_driver.c | 17 +- src/qemu/qemu_hotplug.c | 15 +- src/qemu/qemu_hotplug.h | 9 +- src/secret/secret_driver.c | 8 + src/util/buf.c | 33 ++- src/util/buf.h | 1 + .../qemuxml2argv-disk-drive-network-rbd.args | 6 +- .../qemuxml2argv-disk-drive-network-rbd.xml | 1 + 16 files changed, 305 insertions(+), 156 deletions(-)
On Fri, Oct 07, 2011 at 01:39:50PM +0200, Wido den Hollander wrote:
Hi,
On Mon, 2011-09-19 at 21:13 -0700, Sage Weil wrote:
The current support for qemu and Ceph RBD (rados block device) has two main deficiencies: authentication doesn't work, and it relies on environment variables (which don't work with latest upstream). This patch set addresses both those problems.
Authentication is something that is really needed in the libvirt implementation of RBD.
The interest in RBD is growing rapidly, but without auth support it's kind of useless.
Would like to see this implemented!
Apologies for not reviewing this patch series before now. It accidentally slipped off my todo list. I'll aim to take a look at in at the start of next week 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 :|
On Mon, Sep 19, 2011 at 09:13:38PM -0700, Sage Weil wrote:
The current support for qemu and Ceph RBD (rados block device) has two main deficiencies: authentication doesn't work, and it relies on environment variables (which don't work with latest upstream). This patch set addresses both those problems.
The first two patches update the xml schemas and conf to add a Ceph secret type and to specify authentication information along with the rbd disk.
The next two patches make some libvirt changes. We pass virConnectPtr down into the Domain{Attach,Detach} methods (needed to access secrets while building the qemu command), and add a helper that will escape arbitrary characters.
The final patch replaces the current RBD qemu code and uses the new conf info to do authentication properly. (We still need to make a change there to avoid having the authentication key show up on qemu command line; I'll clean that up shortly.)
Comments on this approach?
Ok, I've finally got myself a Ceph cluster up & running, with RBD exports to a QEMU guest[1], so I can give more sensible answers to these patches :-) Overall you are taking the current XML for a disk: <disk type='network' device='disk'> <driver name='qemu' type='raw'/> <source protocol='rbd' name='demo/wibble'> <host name='lettuce.example.org' port='6798'/> <host name='mustard.example.org' port='6798'/> <host name='avocado.example.org' port='6798'/> </source> <target dev='vdb' bus='virtio'/> </disk> and adding one new element <auth> such that we get <disk type='network' device='disk'> <driver name='qemu' type='raw'/> <source protocol='rbd' name='demo/wibble'> <auth id='admin' domain='clustername'/> <host name='lettuce.example.org' port='6798'/> <host name='mustard.example.org' port='6798'/> <host name='avocado.example.org' port='6798'/> </source> <target dev='vdb' bus='virtio'/> </disk> together with some secret XML that looks like: <secret ephemeral='no' private='no'> <uuid>0a81f5b2-8403-7b23-c8d6-21ccc2f80d6f</uuid> <usage type='ceph'> <auth id='admin' domain='clustername'/> </usage> </secret> When starting a guest the auth id + domain are concatenated to find the secret. The 'id' is also used as the username to pass to QEMU. The 'domain' is only ever used for the secret lookup part, never passed to QEMU, since the host IP addrs do that job. By comparison, QCow2 encryption disks are done with: <disk type='file' device='disk'> <driver name='qemu' type='qcow2'/> <source file='/home/berrange/VirtualMachines/demo.qcow2'/> <target dev='hda' bus='ide'/> <encryption format='qcow'> <secret type='passphrase' uuid='0a81f5b2-8403-7b23-c8d6-21ccc2f80d6f'/> </encryption> </disk> And the secret with: <secret ephemeral='no' private='no'> <uuid>0a81f5b2-8403-7b23-c8d6-21ccc2f80d6f</uuid> <usage type='volume'> <volume>/home/berrange/VirtualMachines/demo.qcow2</volume> </usage> </secret> When starting a guest, the secret UUID is used to find the passphrase. So we have a difference in the way the secrets are linked to the disks between Ceph and QCow2 here. I can see the appeal of doing it the way you have, but at the same time I would like to have consistency with the QCow2 approach, and UUIDs are a stronger identifier IMHO. Also, although the secret XML is exposed auth id + domain as separate attributes in the XML, internally they're processed as a concatenated string, likewise the public API uses a concatenated string form. So I think the XML ought to use that form too, and not split them. Finally, I think the concept of authentication credentials for disks, could be more generally useful than just for Ceph or other network block devs, so I'm somewhat inclined to move it outside the <source> element Thus I would propose a slight variation on what you've done: <disk type='network' device='disk'> <driver name='qemu' type='raw'/> <auth username='admin'> <secret type='passphrase' uuid='0a81f5b2-8403-7b23-c8d6-21ccc2f80d6f'/> </auth> <source protocol='rbd' name='demo/wibble'> <host name='lettuce.example.org' port='6798'/> <host name='mustard.example.org' port='6798'/> <host name='avocado.example.org' port='6798'/> </source> <target dev='vdb' bus='virtio'/> </disk> And in the secret XML: <secret ephemeral='no' private='no'> <uuid>0a81f5b2-8403-7b23-c8d6-21ccc2f80d6f</uuid> <usage type='ceph'> <domain>some.cluster.name/admin</domain> </usage> </secret> Though I think the UUID based lookup should be primary, I would also accept an optional alternate syntax based on usage strings, if you think that's important: <disk type='network' device='disk'> <driver name='qemu' type='raw'/> <auth username='admin'> <secret type='passphrase' usage='some.cluster.name/admin'/> </auth> <source protocol='rbd' name='demo/wibble'> <host name='lettuce.example.org' port='6798'/> <host name='mustard.example.org' port='6798'/> <host name='avocado.example.org' port='6798'/> </source> <target dev='vdb' bus='virtio'/> </disk> Regards, Daniel [1] http://berrange.com/posts/2011/10/12/setting-up-a-ceph-cluster-and-exporting... -- |: 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 :|
Hi Daniel, On Wed, 12 Oct 2011, Daniel P. Berrange wrote:
On Mon, Sep 19, 2011 at 09:13:38PM -0700, Sage Weil wrote:
The current support for qemu and Ceph RBD (rados block device) has two main deficiencies: authentication doesn't work, and it relies on environment variables (which don't work with latest upstream). This patch set addresses both those problems.
The first two patches update the xml schemas and conf to add a Ceph secret type and to specify authentication information along with the rbd disk.
The next two patches make some libvirt changes. We pass virConnectPtr down into the Domain{Attach,Detach} methods (needed to access secrets while building the qemu command), and add a helper that will escape arbitrary characters.
The final patch replaces the current RBD qemu code and uses the new conf info to do authentication properly. (We still need to make a change there to avoid having the authentication key show up on qemu command line; I'll clean that up shortly.)
Comments on this approach?
Ok, I've finally got myself a Ceph cluster up & running, with RBD exports to a QEMU guest[1], so I can give more sensible answers to these patches :-)
Overall you are taking the current XML for a disk:
<disk type='network' device='disk'> <driver name='qemu' type='raw'/> <source protocol='rbd' name='demo/wibble'> <host name='lettuce.example.org' port='6798'/> <host name='mustard.example.org' port='6798'/> <host name='avocado.example.org' port='6798'/> </source> <target dev='vdb' bus='virtio'/> </disk>
and adding one new element <auth> such that we get
<disk type='network' device='disk'> <driver name='qemu' type='raw'/> <source protocol='rbd' name='demo/wibble'> <auth id='admin' domain='clustername'/> <host name='lettuce.example.org' port='6798'/> <host name='mustard.example.org' port='6798'/> <host name='avocado.example.org' port='6798'/> </source> <target dev='vdb' bus='virtio'/> </disk>
together with some secret XML that looks like:
<secret ephemeral='no' private='no'> <uuid>0a81f5b2-8403-7b23-c8d6-21ccc2f80d6f</uuid> <usage type='ceph'> <auth id='admin' domain='clustername'/> </usage> </secret>
When starting a guest the auth id + domain are concatenated to find the secret. The 'id' is also used as the username to pass to QEMU. The 'domain' is only ever used for the secret lookup part, never passed to QEMU, since the host IP addrs do that job.
By comparison, QCow2 encryption disks are done with:
<disk type='file' device='disk'> <driver name='qemu' type='qcow2'/> <source file='/home/berrange/VirtualMachines/demo.qcow2'/> <target dev='hda' bus='ide'/> <encryption format='qcow'> <secret type='passphrase' uuid='0a81f5b2-8403-7b23-c8d6-21ccc2f80d6f'/> </encryption> </disk>
And the secret with:
<secret ephemeral='no' private='no'> <uuid>0a81f5b2-8403-7b23-c8d6-21ccc2f80d6f</uuid> <usage type='volume'> <volume>/home/berrange/VirtualMachines/demo.qcow2</volume> </usage> </secret>
When starting a guest, the secret UUID is used to find the passphrase.
So we have a difference in the way the secrets are linked to the disks between Ceph and QCow2 here.
I can see the appeal of doing it the way you have, but at the same time I would like to have consistency with the QCow2 approach, and UUIDs are a stronger identifier IMHO.
Also, although the secret XML is exposed auth id + domain as separate attributes in the XML, internally they're processed as a concatenated string, likewise the public API uses a concatenated string form. So I think the XML ought to use that form too, and not split them.
Finally, I think the concept of authentication credentials for disks, could be more generally useful than just for Ceph or other network block devs, so I'm somewhat inclined to move it outside the <source> element
Thus I would propose a slight variation on what you've done:
<disk type='network' device='disk'> <driver name='qemu' type='raw'/> <auth username='admin'> <secret type='passphrase' uuid='0a81f5b2-8403-7b23-c8d6-21ccc2f80d6f'/> </auth> <source protocol='rbd' name='demo/wibble'> <host name='lettuce.example.org' port='6798'/> <host name='mustard.example.org' port='6798'/> <host name='avocado.example.org' port='6798'/> </source> <target dev='vdb' bus='virtio'/> </disk>
And in the secret XML:
<secret ephemeral='no' private='no'> <uuid>0a81f5b2-8403-7b23-c8d6-21ccc2f80d6f</uuid> <usage type='ceph'> <domain>some.cluster.name/admin</domain> </usage> </secret>
Though I think the UUID based lookup should be primary, I would also accept an optional alternate syntax based on usage strings, if you think that's important:
<disk type='network' device='disk'> <driver name='qemu' type='raw'/> <auth username='admin'> <secret type='passphrase' usage='some.cluster.name/admin'/> </auth> <source protocol='rbd' name='demo/wibble'> <host name='lettuce.example.org' port='6798'/> <host name='mustard.example.org' port='6798'/> <host name='avocado.example.org' port='6798'/> </source> <target dev='vdb' bus='virtio'/> </disk>
Yes, I like this much better as well. I wasn't particularly happy with how the keys were being identified; this is much cleaner. I'll respin these and resend shortly. Thanks for the review! sage
participants (4)
-
Daniel P. Berrange -
Eric Blake -
Sage Weil -
Wido den Hollander