All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/5 v2] Improve Ceph Qemu+RBD support
@ 2011-09-20  4:13 Sage Weil
  2011-09-20  4:13 ` [PATCH 1/5] secret: add Ceph secret type Sage Weil
                   ` (7 more replies)
  0 siblings, 8 replies; 18+ messages in thread
From: Sage Weil @ 2011-09-20  4:13 UTC (permalink / raw)
  To: libvir-list; +Cc: ceph-devel, veillard, Sage Weil

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


^ permalink raw reply	[flat|nested] 18+ messages in thread

* [PATCH 1/5] secret: add Ceph secret type
  2011-09-20  4:13 [PATCH 0/5 v2] Improve Ceph Qemu+RBD support Sage Weil
@ 2011-09-20  4:13 ` Sage Weil
  2011-10-12 16:36   ` [libvirt] " Daniel P. Berrange
  2011-09-20  4:13 ` [PATCH 2/5] storage: add authId, authDomain to virDomainDiskDef Sage Weil
                   ` (6 subsequent siblings)
  7 siblings, 1 reply; 18+ messages in thread
From: Sage Weil @ 2011-09-20  4:13 UTC (permalink / raw)
  To: libvir-list; +Cc: ceph-devel, veillard, Sage Weil

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


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH 2/5] storage: add authId, authDomain to virDomainDiskDef
  2011-09-20  4:13 [PATCH 0/5 v2] Improve Ceph Qemu+RBD support Sage Weil
  2011-09-20  4:13 ` [PATCH 1/5] secret: add Ceph secret type Sage Weil
@ 2011-09-20  4:13 ` Sage Weil
  2011-10-12 16:38   ` [libvirt] " Daniel P. Berrange
  2011-09-20  4:13 ` [PATCH 3/5] qemu: pass virConnectPtr into Domain{Attach,Detach}* Sage Weil
                   ` (5 subsequent siblings)
  7 siblings, 1 reply; 18+ messages in thread
From: Sage Weil @ 2011-09-20  4:13 UTC (permalink / raw)
  To: libvir-list; +Cc: ceph-devel, veillard, Sage Weil

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


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH 3/5] qemu: pass virConnectPtr into Domain{Attach,Detach}*
  2011-09-20  4:13 [PATCH 0/5 v2] Improve Ceph Qemu+RBD support Sage Weil
  2011-09-20  4:13 ` [PATCH 1/5] secret: add Ceph secret type Sage Weil
  2011-09-20  4:13 ` [PATCH 2/5] storage: add authId, authDomain to virDomainDiskDef Sage Weil
@ 2011-09-20  4:13 ` Sage Weil
  2011-10-12 16:40   ` [libvirt] [PATCH 3/5] qemu: pass virConnectPtr into Domain{Attach, Detach}* Daniel P. Berrange
  2011-09-20  4:13 ` [PATCH 4/5] buf: implement generic virBufferEscape Sage Weil
                   ` (4 subsequent siblings)
  7 siblings, 1 reply; 18+ messages in thread
From: Sage Weil @ 2011-09-20  4:13 UTC (permalink / raw)
  To: libvir-list; +Cc: ceph-devel, veillard, Sage Weil

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


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH 4/5] buf: implement generic virBufferEscape
  2011-09-20  4:13 [PATCH 0/5 v2] Improve Ceph Qemu+RBD support Sage Weil
                   ` (2 preceding siblings ...)
  2011-09-20  4:13 ` [PATCH 3/5] qemu: pass virConnectPtr into Domain{Attach,Detach}* Sage Weil
@ 2011-09-20  4:13 ` Sage Weil
  2011-10-11 10:39   ` [libvirt] " Daniel P. Berrange
  2011-09-20  4:13 ` [PATCH 5/5] qemu/rbd: improve rbd device specification Sage Weil
                   ` (3 subsequent siblings)
  7 siblings, 1 reply; 18+ messages in thread
From: Sage Weil @ 2011-09-20  4:13 UTC (permalink / raw)
  To: libvir-list; +Cc: ceph-devel, veillard, Sage Weil

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


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH 5/5] qemu/rbd: improve rbd device specification
  2011-09-20  4:13 [PATCH 0/5 v2] Improve Ceph Qemu+RBD support Sage Weil
                   ` (3 preceding siblings ...)
  2011-09-20  4:13 ` [PATCH 4/5] buf: implement generic virBufferEscape Sage Weil
@ 2011-09-20  4:13 ` Sage Weil
  2011-10-12 16:48   ` [libvirt] " Daniel P. Berrange
  2011-09-27 20:40 ` [PATCH 0/5 v2] Improve Ceph Qemu+RBD support Sage Weil
                   ` (2 subsequent siblings)
  7 siblings, 1 reply; 18+ messages in thread
From: Sage Weil @ 2011-09-20  4:13 UTC (permalink / raw)
  To: libvir-list; +Cc: ceph-devel, veillard, Sage Weil

This improves the support for qemu rbd devices by adding support for a few
key features (e.g., authentication) and cleaning up the way in which
rbd configuration options are passed to qemu.

And <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


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* Re: [PATCH 0/5 v2] Improve Ceph Qemu+RBD support
  2011-09-20  4:13 [PATCH 0/5 v2] Improve Ceph Qemu+RBD support Sage Weil
                   ` (4 preceding siblings ...)
  2011-09-20  4:13 ` [PATCH 5/5] qemu/rbd: improve rbd device specification Sage Weil
@ 2011-09-27 20:40 ` Sage Weil
  2011-10-07 11:39 ` Wido den Hollander
  2011-10-12 16:33 ` Daniel P. Berrange
  7 siblings, 0 replies; 18+ messages in thread
From: Sage Weil @ 2011-09-27 20:40 UTC (permalink / raw)
  To: libvir-list; +Cc: ceph-devel, veillard

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
> 
> 

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 0/5 v2] Improve Ceph Qemu+RBD support
  2011-09-20  4:13 [PATCH 0/5 v2] Improve Ceph Qemu+RBD support Sage Weil
                   ` (5 preceding siblings ...)
  2011-09-27 20:40 ` [PATCH 0/5 v2] Improve Ceph Qemu+RBD support Sage Weil
@ 2011-10-07 11:39 ` Wido den Hollander
  2011-10-07 12:17   ` [libvirt] " Daniel P. Berrange
  2011-10-12 16:33 ` Daniel P. Berrange
  7 siblings, 1 reply; 18+ messages in thread
From: Wido den Hollander @ 2011-10-07 11:39 UTC (permalink / raw)
  To: Sage Weil; +Cc: libvir-list, ceph-devel, veillard

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(-)
> 



^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [libvirt] [PATCH 0/5 v2] Improve Ceph Qemu+RBD support
  2011-10-07 11:39 ` Wido den Hollander
@ 2011-10-07 12:17   ` Daniel P. Berrange
  0 siblings, 0 replies; 18+ messages in thread
From: Daniel P. Berrange @ 2011-10-07 12:17 UTC (permalink / raw)
  To: Wido den Hollander; +Cc: Sage Weil, libvir-list, ceph-devel

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 :|

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [libvirt] [PATCH 4/5] buf: implement generic virBufferEscape
  2011-09-20  4:13 ` [PATCH 4/5] buf: implement generic virBufferEscape Sage Weil
@ 2011-10-11 10:39   ` Daniel P. Berrange
  2011-10-12 17:09     ` Eric Blake
  0 siblings, 1 reply; 18+ messages in thread
From: Daniel P. Berrange @ 2011-10-11 10:39 UTC (permalink / raw)
  To: Sage Weil; +Cc: libvir-list, ceph-devel

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 :|

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [libvirt] [PATCH 0/5 v2] Improve Ceph Qemu+RBD support
  2011-09-20  4:13 [PATCH 0/5 v2] Improve Ceph Qemu+RBD support Sage Weil
                   ` (6 preceding siblings ...)
  2011-10-07 11:39 ` Wido den Hollander
@ 2011-10-12 16:33 ` Daniel P. Berrange
  2011-10-12 16:45   ` Sage Weil
  7 siblings, 1 reply; 18+ messages in thread
From: Daniel P. Berrange @ 2011-10-12 16:33 UTC (permalink / raw)
  To: Sage Weil; +Cc: libvir-list, ceph-devel

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-a-rbd-volume-to-a-kvm-guest/
-- 
|: 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 :|

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [libvirt] [PATCH 1/5] secret: add Ceph secret type
  2011-09-20  4:13 ` [PATCH 1/5] secret: add Ceph secret type Sage Weil
@ 2011-10-12 16:36   ` Daniel P. Berrange
  0 siblings, 0 replies; 18+ messages in thread
From: Daniel P. Berrange @ 2011-10-12 16:36 UTC (permalink / raw)
  To: Sage Weil; +Cc: libvir-list, ceph-devel

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 :|

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [libvirt] [PATCH 2/5] storage: add authId, authDomain to virDomainDiskDef
  2011-09-20  4:13 ` [PATCH 2/5] storage: add authId, authDomain to virDomainDiskDef Sage Weil
@ 2011-10-12 16:38   ` Daniel P. Berrange
  0 siblings, 0 replies; 18+ messages in thread
From: Daniel P. Berrange @ 2011-10-12 16:38 UTC (permalink / raw)
  To: Sage Weil; +Cc: libvir-list, ceph-devel

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 :|

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [libvirt] [PATCH 3/5] qemu: pass virConnectPtr into Domain{Attach, Detach}*
  2011-09-20  4:13 ` [PATCH 3/5] qemu: pass virConnectPtr into Domain{Attach,Detach}* Sage Weil
@ 2011-10-12 16:40   ` Daniel P. Berrange
  0 siblings, 0 replies; 18+ messages in thread
From: Daniel P. Berrange @ 2011-10-12 16:40 UTC (permalink / raw)
  To: Sage Weil; +Cc: libvir-list, ceph-devel

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 :|

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [libvirt] [PATCH 0/5 v2] Improve Ceph Qemu+RBD support
  2011-10-12 16:33 ` Daniel P. Berrange
@ 2011-10-12 16:45   ` Sage Weil
  0 siblings, 0 replies; 18+ messages in thread
From: Sage Weil @ 2011-10-12 16:45 UTC (permalink / raw)
  To: Daniel P. Berrange; +Cc: libvir-list, ceph-devel

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

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [libvirt] [PATCH 5/5] qemu/rbd: improve rbd device specification
  2011-09-20  4:13 ` [PATCH 5/5] qemu/rbd: improve rbd device specification Sage Weil
@ 2011-10-12 16:48   ` Daniel P. Berrange
  0 siblings, 0 replies; 18+ messages in thread
From: Daniel P. Berrange @ 2011-10-12 16:48 UTC (permalink / raw)
  To: Sage Weil; +Cc: libvir-list, ceph-devel

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 :|

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [libvirt] [PATCH 4/5] buf: implement generic virBufferEscape
  2011-10-11 10:39   ` [libvirt] " Daniel P. Berrange
@ 2011-10-12 17:09     ` Eric Blake
  0 siblings, 0 replies; 18+ messages in thread
From: Eric Blake @ 2011-10-12 17:09 UTC (permalink / raw)
  To: Daniel P. Berrange; +Cc: Sage Weil, libvir-list, ceph-devel

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

^ permalink raw reply related	[flat|nested] 18+ messages in thread

* Re: [libvirt] [PATCH 1/5] secret: add Ceph secret type
  2011-09-15 20:52 ` [PATCH 1/5] secret: add Ceph secret type Sage Weil
@ 2011-09-16  8:18   ` Osier Yang
  0 siblings, 0 replies; 18+ messages in thread
From: Osier Yang @ 2011-09-16  8:18 UTC (permalink / raw)
  To: Sage Weil; +Cc: libvir-list, ceph-devel

于 2011年09月16日 04:52, Sage Weil 写道:
> 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>
> ---
>   include/libvirt/libvirt.h.in |    3 ++
>   src/conf/secret_conf.c       |   45 +++++++++++++++++++++++++++++++++++++++++-
>   src/conf/secret_conf.h       |    1 +
>   src/secret/secret_driver.c   |    8 +++++++
>   4 files changed, 56 insertions(+), 1 deletions(-)
>
> 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);

Should use VIR_ALLOC/VIR_ALLOC_N here.

> +        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;
>       }

--
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

^ permalink raw reply	[flat|nested] 18+ messages in thread

end of thread, other threads:[~2011-10-12 17:09 UTC | newest]

Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-09-20  4:13 [PATCH 0/5 v2] Improve Ceph Qemu+RBD support Sage Weil
2011-09-20  4:13 ` [PATCH 1/5] secret: add Ceph secret type Sage Weil
2011-10-12 16:36   ` [libvirt] " Daniel P. Berrange
2011-09-20  4:13 ` [PATCH 2/5] storage: add authId, authDomain to virDomainDiskDef Sage Weil
2011-10-12 16:38   ` [libvirt] " Daniel P. Berrange
2011-09-20  4:13 ` [PATCH 3/5] qemu: pass virConnectPtr into Domain{Attach,Detach}* Sage Weil
2011-10-12 16:40   ` [libvirt] [PATCH 3/5] qemu: pass virConnectPtr into Domain{Attach, Detach}* Daniel P. Berrange
2011-09-20  4:13 ` [PATCH 4/5] buf: implement generic virBufferEscape Sage Weil
2011-10-11 10:39   ` [libvirt] " Daniel P. Berrange
2011-10-12 17:09     ` Eric Blake
2011-09-20  4:13 ` [PATCH 5/5] qemu/rbd: improve rbd device specification Sage Weil
2011-10-12 16:48   ` [libvirt] " Daniel P. Berrange
2011-09-27 20:40 ` [PATCH 0/5 v2] Improve Ceph Qemu+RBD support Sage Weil
2011-10-07 11:39 ` Wido den Hollander
2011-10-07 12:17   ` [libvirt] " Daniel P. Berrange
2011-10-12 16:33 ` Daniel P. Berrange
2011-10-12 16:45   ` Sage Weil
  -- strict thread matches above, loose matches on Subject: below --
2011-09-15 20:52 [PATCH 0/5] " Sage Weil
2011-09-15 20:52 ` [PATCH 1/5] secret: add Ceph secret type Sage Weil
2011-09-16  8:18   ` [libvirt] " Osier Yang

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.