* [RFC PATCH v3 1/4] secret: add Ceph secret type
2011-10-20 18:01 [RFC PATCH v3 0/4] Improve Ceph Qemu+RBD support Josh Durgin
@ 2011-10-20 18:01 ` Josh Durgin
2011-10-27 8:28 ` Daniel P. Berrange
2011-10-20 18:01 ` [RFC PATCH v3 2/4] storage: add auth to virDomainDiskDef Josh Durgin
` (3 subsequent siblings)
4 siblings, 1 reply; 27+ messages in thread
From: Josh Durgin @ 2011-10-20 18:01 UTC (permalink / raw)
To: libvir-list; +Cc: ceph-devel, berrange
From: Sage Weil <sage@newdream.net>
Add a new secret type to store a Ceph authentication key. The name
is simply an identifier for easy human reference.
The xml looks like this:
<secret ephemeral='no' private='no'>
<uuid>0a81f5b2-8403-7b23-c8d6-21ccc2f80d6f</uuid>
<usage type='ceph'>
<name>mycluster_admin</name>
</usage>
</secret>
Signed-off-by: Sage Weil <sage@newdream.net>
Signed-off-by: Josh Durgin <josh.durgin@dreamhost.com>
---
docs/schemas/secret.rng | 10 ++++++++++
include/libvirt/libvirt.h.in | 3 +++
src/conf/secret_conf.c | 23 ++++++++++++++++++++++-
src/conf/secret_conf.h | 1 +
src/secret/secret_driver.c | 8 ++++++++
5 files changed, 44 insertions(+), 1 deletions(-)
diff --git a/docs/schemas/secret.rng b/docs/schemas/secret.rng
index 80270ae..8e7714b 100644
--- a/docs/schemas/secret.rng
+++ b/docs/schemas/secret.rng
@@ -37,6 +37,7 @@
<element name='usage'>
<choice>
<ref name='usagevolume'/>
+ <ref name='usageceph'/>
<!-- More choices later -->
</choice>
</element>
@@ -54,6 +55,15 @@
</element>
</define>
+ <define name='usageceph'>
+ <attribute name='type'>
+ <value>ceph</value>
+ </attribute>
+ <element name='name'>
+ <text/>
+ </element>
+ </define>
+
<define name="UUID">
<choice>
<data type="string">
diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in
index 361881a..2ef1c9c 100644
--- a/include/libvirt/libvirt.h.in
+++ b/include/libvirt/libvirt.h.in
@@ -2381,7 +2381,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 b33ce98..a51fc69 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.ceph);
+ break;
+
default:
VIR_ERROR(_("unexpected secret usage type %d"), def->usage_type);
break;
@@ -94,6 +99,15 @@ virSecretDefParseUsage(xmlXPathContextPtr ctxt,
}
break;
+ case VIR_SECRET_USAGE_TYPE_CEPH:
+ def->usage.ceph = virXPathString("string(./usage/name)", ctxt);
+ if (!def->usage.ceph) {
+ virSecretReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("Ceph usage specified, but name is missing"));
+ return -1;
+ }
+ break;
+
default:
virSecretReportError(VIR_ERR_INTERNAL_ERROR,
_("unexpected secret usage type %d"),
@@ -239,6 +253,13 @@ virSecretDefFormatUsage(virBufferPtr buf,
def->usage.volume);
break;
+ case VIR_SECRET_USAGE_TYPE_CEPH:
+ if (def->usage.ceph != NULL) {
+ virBufferEscapeString(buf, " <name>%s</name>\n",
+ def->usage.ceph);
+ }
+ 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..b5d72d4 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 *ceph;
} usage;
};
diff --git a/src/secret/secret_driver.c b/src/secret/secret_driver.c
index 59dc687..088a243 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.ceph, 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.ceph;
+
default:
return NULL;
}
--
1.7.1
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [RFC PATCH v3 1/4] secret: add Ceph secret type
2011-10-20 18:01 ` [RFC PATCH v3 1/4] secret: add Ceph secret type Josh Durgin
@ 2011-10-27 8:28 ` Daniel P. Berrange
2011-10-28 16:33 ` [libvirt] " Eric Blake
2011-10-28 17:41 ` Eric Blake
0 siblings, 2 replies; 27+ messages in thread
From: Daniel P. Berrange @ 2011-10-27 8:28 UTC (permalink / raw)
To: Josh Durgin; +Cc: libvir-list, ceph-devel
On Thu, Oct 20, 2011 at 11:01:24AM -0700, Josh Durgin wrote:
> From: Sage Weil <sage@newdream.net>
>
> Add a new secret type to store a Ceph authentication key. The name
> is simply an identifier for easy human reference.
>
> The xml looks like this:
>
> <secret ephemeral='no' private='no'>
> <uuid>0a81f5b2-8403-7b23-c8d6-21ccc2f80d6f</uuid>
> <usage type='ceph'>
> <name>mycluster_admin</name>
> </usage>
> </secret>
>
> Signed-off-by: Sage Weil <sage@newdream.net>
> Signed-off-by: Josh Durgin <josh.durgin@dreamhost.com>
> ---
> docs/schemas/secret.rng | 10 ++++++++++
> include/libvirt/libvirt.h.in | 3 +++
> src/conf/secret_conf.c | 23 ++++++++++++++++++++++-
> src/conf/secret_conf.h | 1 +
> src/secret/secret_driver.c | 8 ++++++++
> 5 files changed, 44 insertions(+), 1 deletions(-)
ACK
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] 27+ messages in thread
* Re: [libvirt] [RFC PATCH v3 1/4] secret: add Ceph secret type
2011-10-27 8:28 ` Daniel P. Berrange
@ 2011-10-28 16:33 ` Eric Blake
2011-10-28 16:42 ` Eric Blake
2011-10-28 17:41 ` Eric Blake
1 sibling, 1 reply; 27+ messages in thread
From: Eric Blake @ 2011-10-28 16:33 UTC (permalink / raw)
To: Daniel P. Berrange; +Cc: Josh Durgin, libvir-list, ceph-devel
On 10/27/2011 02:28 AM, Daniel P. Berrange wrote:
> On Thu, Oct 20, 2011 at 11:01:24AM -0700, Josh Durgin wrote:
>> From: Sage Weil<sage@newdream.net>
>>
>> Add a new secret type to store a Ceph authentication key. The name
>> is simply an identifier for easy human reference.
>>
>> The xml looks like this:
>>
>> <secret ephemeral='no' private='no'>
>> <uuid>0a81f5b2-8403-7b23-c8d6-21ccc2f80d6f</uuid>
>> <usage type='ceph'>
>> <name>mycluster_admin</name>
>> </usage>
>> </secret>
>>
>> Signed-off-by: Sage Weil<sage@newdream.net>
>> Signed-off-by: Josh Durgin<josh.durgin@dreamhost.com>
>> ---
>> docs/schemas/secret.rng | 10 ++++++++++
>> include/libvirt/libvirt.h.in | 3 +++
>> src/conf/secret_conf.c | 23 ++++++++++++++++++++++-
>> src/conf/secret_conf.h | 1 +
>> src/secret/secret_driver.c | 8 ++++++++
>> 5 files changed, 44 insertions(+), 1 deletions(-)
>
> ACK
I can't find the mail you are replying to, in order to apply it to the
libvirt tree; was it accidentally sent off-list? Can anyone point me to
a git tree with this patch series ready to be merged, or else resend it?
--
Eric Blake eblake@redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [libvirt] [RFC PATCH v3 1/4] secret: add Ceph secret type
2011-10-28 16:33 ` [libvirt] " Eric Blake
@ 2011-10-28 16:42 ` Eric Blake
0 siblings, 0 replies; 27+ messages in thread
From: Eric Blake @ 2011-10-28 16:42 UTC (permalink / raw)
Cc: Daniel P. Berrange, libvir-list, ceph-devel, Josh Durgin
On 10/28/2011 10:33 AM, Eric Blake wrote:
> On 10/27/2011 02:28 AM, Daniel P. Berrange wrote:
>> On Thu, Oct 20, 2011 at 11:01:24AM -0700, Josh Durgin wrote:
>>> From: Sage Weil<sage@newdream.net>
>>>
>>> Add a new secret type to store a Ceph authentication key. The name
>>> is simply an identifier for easy human reference.
>>>
>>> The xml looks like this:
>>>
>>> <secret ephemeral='no' private='no'>
>>> <uuid>0a81f5b2-8403-7b23-c8d6-21ccc2f80d6f</uuid>
>>> <usage type='ceph'>
>>> <name>mycluster_admin</name>
>>> </usage>
>>> </secret>
>>>
>>> Signed-off-by: Sage Weil<sage@newdream.net>
>>> Signed-off-by: Josh Durgin<josh.durgin@dreamhost.com>
>>> ---
>>> docs/schemas/secret.rng | 10 ++++++++++
>>> include/libvirt/libvirt.h.in | 3 +++
>>> src/conf/secret_conf.c | 23 ++++++++++++++++++++++-
>>> src/conf/secret_conf.h | 1 +
>>> src/secret/secret_driver.c | 8 ++++++++
>>> 5 files changed, 44 insertions(+), 1 deletions(-)
>>
>> ACK
>
> I can't find the mail you are replying to, in order to apply it to the
> libvirt tree; was it accidentally sent off-list? Can anyone point me to
> a git tree with this patch series ready to be merged, or else resend it?
OK, I see an archive of it here:
http://thread.gmane.org/gmane.comp.file-systems.ceph.devel/4129/focus=4133
but mail archives tend to corrupt any text with @, so I hope I'm porting
it correctly. I'm not sure why the mail showed up on ceph-devel but not
libvir-list.
--
Eric Blake eblake@redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [libvirt] [RFC PATCH v3 1/4] secret: add Ceph secret type
2011-10-27 8:28 ` Daniel P. Berrange
2011-10-28 16:33 ` [libvirt] " Eric Blake
@ 2011-10-28 17:41 ` Eric Blake
2011-10-28 18:47 ` Josh Durgin
1 sibling, 1 reply; 27+ messages in thread
From: Eric Blake @ 2011-10-28 17:41 UTC (permalink / raw)
To: Daniel P. Berrange; +Cc: Josh Durgin, libvir-list, ceph-devel
On 10/27/2011 02:28 AM, Daniel P. Berrange wrote:
> On Thu, Oct 20, 2011 at 11:01:24AM -0700, Josh Durgin wrote:
>> From: Sage Weil<sage@newdream.net>
>>
>> Add a new secret type to store a Ceph authentication key. The name
>> is simply an identifier for easy human reference.
>>
>> The xml looks like this:
>>
>> <secret ephemeral='no' private='no'>
>> <uuid>0a81f5b2-8403-7b23-c8d6-21ccc2f80d6f</uuid>
>> <usage type='ceph'>
>> <name>mycluster_admin</name>
>> </usage>
>> </secret>
>>
>> Signed-off-by: Sage Weil<sage@newdream.net>
>> Signed-off-by: Josh Durgin<josh.durgin@dreamhost.com>
>> ---
>> docs/schemas/secret.rng | 10 ++++++++++
Missing docs/formatsecret.html.in changes to document this, but I think
I managed.
>> include/libvirt/libvirt.h.in | 3 +++
>> src/conf/secret_conf.c | 23 ++++++++++++++++++++++-
>> src/conf/secret_conf.h | 1 +
>> src/secret/secret_driver.c | 8 ++++++++
>> 5 files changed, 44 insertions(+), 1 deletions(-)
>
> ACK
I'm adding this, and pushing:
diff --git i/docs/formatsecret.html.in w/docs/formatsecret.html.in
index 63a1f2a..01aff2d 100644
--- i/docs/formatsecret.html.in
+++ w/docs/formatsecret.html.in
@@ -39,8 +39,8 @@
<dd>
Specifies what this secret is used for. A mandatory
<code>type</code> attribute specifies the usage category,
currently
- only <code>volume</code> is defined. Specific usage categories are
- described below.
+ only <code>volume</code> and <code>ceph</code> are defined.
+ Specific usage categories are described below.
</dd>
</dl>
@@ -54,6 +54,18 @@
this secret is associated with.
</p>
+ <h3>Usage type "ceph"</h3>
+
+ <p>
+ This secret is associated with a Ceph RBD (rados block device).
+ The <code><usage type='ceph'></code> element must contain
+ a single <code>name</code> element that specifies a usage name
+ for the secret. The Ceph secret can then be used by UUID or by
+ this usage name via the <code><auth></code> element of
+ a <a href="domain.html#elementsDisks">disk
+ device</a>. <span class="since">Since 0.9.7</span>.
+ </p>
+
<h2><a name="example">Example</a></h2>
<pre>
--
Eric Blake eblake@redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [libvirt] [RFC PATCH v3 1/4] secret: add Ceph secret type
2011-10-28 17:41 ` Eric Blake
@ 2011-10-28 18:47 ` Josh Durgin
2011-10-28 18:56 ` Eric Blake
0 siblings, 1 reply; 27+ messages in thread
From: Josh Durgin @ 2011-10-28 18:47 UTC (permalink / raw)
To: Eric Blake; +Cc: Daniel P. Berrange, libvir-list, ceph-devel
On 10/28/2011 10:41 AM, Eric Blake wrote:
> On 10/27/2011 02:28 AM, Daniel P. Berrange wrote:
>> On Thu, Oct 20, 2011 at 11:01:24AM -0700, Josh Durgin wrote:
>>> From: Sage Weil<sage@newdream.net>
>>>
>>> Add a new secret type to store a Ceph authentication key. The name
>>> is simply an identifier for easy human reference.
>>>
>>> The xml looks like this:
>>>
>>> <secret ephemeral='no' private='no'>
>>> <uuid>0a81f5b2-8403-7b23-c8d6-21ccc2f80d6f</uuid>
>>> <usage type='ceph'>
>>> <name>mycluster_admin</name>
>>> </usage>
>>> </secret>
>>>
>>> Signed-off-by: Sage Weil<sage@newdream.net>
>>> Signed-off-by: Josh Durgin<josh.durgin@dreamhost.com>
>>> ---
>>> docs/schemas/secret.rng | 10 ++++++++++
>
> Missing docs/formatsecret.html.in changes to document this, but I think
> I managed.
>
>>> include/libvirt/libvirt.h.in | 3 +++
>>> src/conf/secret_conf.c | 23 ++++++++++++++++++++++-
>>> src/conf/secret_conf.h | 1 +
>>> src/secret/secret_driver.c | 8 ++++++++
>>> 5 files changed, 44 insertions(+), 1 deletions(-)
>>
>> ACK
>
> I'm adding this, and pushing:
Thanks, I'm not sure why the mail didn't go through to the libvirt list.
It looks like there's a break missing in the pushed version though:
diff --git a/src/conf/secret_conf.c b/src/conf/secret_conf.c
index fa80888..a51fc69 100644
--- a/src/conf/secret_conf.c
+++ b/src/conf/secret_conf.c
@@ -55,6 +55,7 @@ virSecretDefFree(virSecretDefPtr def)
case VIR_SECRET_USAGE_TYPE_CEPH:
VIR_FREE(def->usage.ceph);
+ break;
default:
VIR_ERROR(_("unexpected secret usage type %d"), def->usage_type);
I'll send an updated version of the other patches shortly.
>
> diff --git i/docs/formatsecret.html.in w/docs/formatsecret.html.in
> index 63a1f2a..01aff2d 100644
> --- i/docs/formatsecret.html.in
> +++ w/docs/formatsecret.html.in
> @@ -39,8 +39,8 @@
> <dd>
> Specifies what this secret is used for. A mandatory
> <code>type</code> attribute specifies the usage category, currently
> - only <code>volume</code> is defined. Specific usage categories are
> - described below.
> + only <code>volume</code> and <code>ceph</code> are defined.
> + Specific usage categories are described below.
> </dd>
> </dl>
>
> @@ -54,6 +54,18 @@
> this secret is associated with.
> </p>
>
> + <h3>Usage type "ceph"</h3>
> +
> + <p>
> + This secret is associated with a Ceph RBD (rados block device).
> + The <code><usage type='ceph'></code> element must contain
> + a single <code>name</code> element that specifies a usage name
> + for the secret. The Ceph secret can then be used by UUID or by
> + this usage name via the <code><auth></code> element of
> + a <a href="domain.html#elementsDisks">disk
> + device</a>. <span class="since">Since 0.9.7</span>.
> + </p>
> +
> <h2><a name="example">Example</a></h2>
>
> <pre>
>
>
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [libvirt] [RFC PATCH v3 1/4] secret: add Ceph secret type
2011-10-28 18:47 ` Josh Durgin
@ 2011-10-28 18:56 ` Eric Blake
0 siblings, 0 replies; 27+ messages in thread
From: Eric Blake @ 2011-10-28 18:56 UTC (permalink / raw)
To: Josh Durgin; +Cc: Daniel P. Berrange, libvir-list, ceph-devel
On 10/28/2011 12:47 PM, Josh Durgin wrote:
>
> Thanks, I'm not sure why the mail didn't go through to the libvirt list.
> It looks like there's a break missing in the pushed version though:
>
> diff --git a/src/conf/secret_conf.c b/src/conf/secret_conf.c
> index fa80888..a51fc69 100644
> --- a/src/conf/secret_conf.c
> +++ b/src/conf/secret_conf.c
> @@ -55,6 +55,7 @@ virSecretDefFree(virSecretDefPtr def)
>
> case VIR_SECRET_USAGE_TYPE_CEPH:
> VIR_FREE(def->usage.ceph);
> + break;
Apologies; that's what I get for manually applying your patch. :(
I've pushed a trivial fix for that...
--
Eric Blake eblake@redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
^ permalink raw reply [flat|nested] 27+ messages in thread
* [RFC PATCH v3 2/4] storage: add auth to virDomainDiskDef
2011-10-20 18:01 [RFC PATCH v3 0/4] Improve Ceph Qemu+RBD support Josh Durgin
2011-10-20 18:01 ` [RFC PATCH v3 1/4] secret: add Ceph secret type Josh Durgin
@ 2011-10-20 18:01 ` Josh Durgin
2011-10-27 8:33 ` Daniel P. Berrange
2011-10-20 18:01 ` [RFC PATCH v3 3/4] qemu: pass virConnectPtr into Domain{Attach,Detach}* Josh Durgin
` (2 subsequent siblings)
4 siblings, 1 reply; 27+ messages in thread
From: Josh Durgin @ 2011-10-20 18:01 UTC (permalink / raw)
To: libvir-list; +Cc: ceph-devel, berrange
Add additional fields to let you specify the how to authenticate with a disk.
The secret to use may be referenced by a usage string or a UUID, i.e.:
<auth username='myuser'>
<secret type='ceph' usage='secretname'/>
</auth>
or
<auth username='myuser'>
<secret type='ceph' uuid='0a81f5b2-8403-7b23-c8d6-21ccc2f80d6f'/>
</auth>
Signed-off-by: Josh Durgin <josh.durgin@dreamhost.com>
---
docs/schemas/domaincommon.rng | 29 +++++++++++
src/Makefile.am | 3 +-
src/conf/domain_conf.c | 105 +++++++++++++++++++++++++++++++++++++---
src/conf/domain_conf.h | 17 +++++++
4 files changed, 145 insertions(+), 9 deletions(-)
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index cd1a067..dda4063 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -602,6 +602,9 @@
<optional>
<ref name="driver"/>
</optional>
+ <optional>
+ <ref name="diskAuth"/>
+ </optional>
<ref name="target"/>
<optional>
<ref name="deviceBoot"/>
@@ -2519,6 +2522,32 @@
<empty/>
</element>
</define>
+ <define name="diskAuth">
+ <element name="auth">
+ <attribute name="username">
+ <ref name="genericName"/>
+ </attribute>
+ <ref name="diskAuthSecret"/>
+ </element>
+ </define>
+
+ <define name='diskAuthSecret'>
+ <element name='secret'>
+ <attribute name='type'>
+ <choice>
+ <value>ceph</value>
+ </choice>
+ </attribute>
+ <choice>
+ <attribute name='uuid'>
+ <ref name="UUID"/>
+ </attribute>
+ <attribute name="usage">
+ <ref name="genericName"/>
+ </attribute>
+ </choice>
+ </element>
+ </define>
<!--
Optional hypervisor extensions in their own namespace:
diff --git a/src/Makefile.am b/src/Makefile.am
index 2555f81..7f48981 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -128,7 +128,8 @@ DOMAIN_CONF_SOURCES = \
conf/capabilities.c conf/capabilities.h \
conf/domain_conf.c conf/domain_conf.h \
conf/domain_audit.c conf/domain_audit.h \
- conf/domain_nwfilter.c conf/domain_nwfilter.h
+ conf/domain_nwfilter.c conf/domain_nwfilter.h \
+ conf/secret_conf.c
DOMAIN_EVENT_SOURCES = \
conf/domain_event.c conf/domain_event.h
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 5959593..1de3742 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -49,6 +49,7 @@
#include "virfile.h"
#include "bitmap.h"
#include "count-one-bits.h"
+#include "secret_conf.h"
#define VIR_FROM_THIS VIR_FROM_DOMAIN
@@ -185,6 +186,11 @@ VIR_ENUM_IMPL(virDomainDiskProtocol, VIR_DOMAIN_DISK_PROTOCOL_LAST,
"rbd",
"sheepdog")
+VIR_ENUM_IMPL(virDomainDiskSecretType, VIR_DOMAIN_DISK_SECRET_TYPE_LAST,
+ "none",
+ "uuid",
+ "usage")
+
VIR_ENUM_IMPL(virDomainDiskIo, VIR_DOMAIN_DISK_IO_LAST,
"default",
"native",
@@ -782,6 +788,9 @@ void virDomainDiskDefFree(virDomainDiskDefPtr def)
VIR_FREE(def->dst);
VIR_FREE(def->driverName);
VIR_FREE(def->driverType);
+ VIR_FREE(def->auth.username);
+ if (def->auth.secretType == VIR_DOMAIN_DISK_SECRET_TYPE_USAGE)
+ VIR_FREE(def->auth.secret.usage);
virStorageEncryptionFree(def->encryption);
virDomainDeviceInfoClear(&def->info);
@@ -2298,7 +2307,7 @@ virDomainDiskDefParseXML(virCapsPtr caps,
unsigned int flags)
{
virDomainDiskDefPtr def;
- xmlNodePtr cur, host;
+ xmlNodePtr cur, child;
char *type = NULL;
char *device = NULL;
char *snapshot = NULL;
@@ -2319,6 +2328,10 @@ virDomainDiskDefParseXML(virCapsPtr caps,
char *devaddr = NULL;
virStorageEncryptionPtr encryption = NULL;
char *serial = NULL;
+ char *authUsername = NULL;
+ char *authUsage = NULL;
+ char *authUUID = NULL;
+ char *usageType = NULL;
if (VIR_ALLOC(def) < 0) {
virReportOOMError();
@@ -2374,10 +2387,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;
@@ -2386,20 +2399,20 @@ 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;
+ child = child->next;
}
break;
default:
@@ -2436,6 +2449,58 @@ virDomainDiskDefParseXML(virCapsPtr caps,
iotag = virXMLPropString(cur, "io");
ioeventfd = virXMLPropString(cur, "ioeventfd");
event_idx = virXMLPropString(cur, "event_idx");
+ } else if (xmlStrEqual(cur->name, BAD_CAST "auth")) {
+ authUsername = virXMLPropString(cur, "username");
+ if (authUsername == NULL) {
+ virDomainReportError(VIR_ERR_INTERNAL_ERROR,
+ _("missing username for auth"));
+ goto error;
+ }
+
+ def->auth.secretType = VIR_DOMAIN_DISK_SECRET_TYPE_NONE;
+ child = cur->children;
+ while (child != NULL) {
+ if (child->type == XML_ELEMENT_NODE &&
+ xmlStrEqual(child->name, BAD_CAST "secret")) {
+ usageType = virXMLPropString(child, "type");
+ if (usageType == NULL) {
+ virDomainReportError(VIR_ERR_XML_ERROR,
+ _("missing type for secret"));
+ goto error;
+ }
+ if (virSecretUsageTypeTypeFromString(usageType) !=
+ VIR_SECRET_USAGE_TYPE_CEPH) {
+ virDomainReportError(VIR_ERR_XML_ERROR,
+ _("invalid secret type %s"),
+ usageType);
+ goto error;
+ }
+
+ authUUID = virXMLPropString(child, "uuid");
+ authUsage = virXMLPropString(child, "usage");
+
+ if (authUUID != NULL && authUsage != NULL) {
+ virDomainReportError(VIR_ERR_XML_ERROR,
+ _("only one of uuid and usage can be specfied"));
+ goto error;
+ }
+ if (authUUID != NULL) {
+ def->auth.secretType = VIR_DOMAIN_DISK_SECRET_TYPE_UUID;
+ if (virUUIDParse(authUUID,
+ def->auth.secret.uuid) < 0) {
+ virDomainReportError(VIR_ERR_XML_ERROR,
+ _("malformed uuid %s"),
+ authUUID);
+ goto error;
+ }
+ } else if (authUsage != NULL) {
+ def->auth.secretType = VIR_DOMAIN_DISK_SECRET_TYPE_USAGE;
+ def->auth.secret.usage = authUsage;
+ authUsage = NULL;
+ }
+ }
+ child = child->next;
+ }
} else if (xmlStrEqual(cur->name, BAD_CAST "readonly")) {
def->readonly = 1;
} else if (xmlStrEqual(cur->name, BAD_CAST "shareable")) {
@@ -2654,6 +2719,8 @@ virDomainDiskDefParseXML(virCapsPtr caps,
hosts = NULL;
def->nhosts = nhosts;
nhosts = 0;
+ def->auth.username = authUsername;
+ authUsername = NULL;
def->driverName = driverName;
driverName = NULL;
def->driverType = driverType;
@@ -2690,6 +2757,10 @@ cleanup:
VIR_FREE(hosts);
VIR_FREE(protocol);
VIR_FREE(device);
+ VIR_FREE(authUsername);
+ VIR_FREE(usageType);
+ VIR_FREE(authUUID);
+ VIR_FREE(authUsage);
VIR_FREE(driverType);
VIR_FREE(driverName);
VIR_FREE(cachetag);
@@ -9176,6 +9247,7 @@ virDomainDiskDefFormat(virBufferPtr buf,
const char *iomode = virDomainDiskIoTypeToString(def->iomode);
const char *ioeventfd = virDomainIoEventFdTypeToString(def->ioeventfd);
const char *event_idx = virDomainVirtioEventIdxTypeToString(def->event_idx);
+ char uuidstr[VIR_UUID_STRING_BUFLEN];
if (!type) {
virDomainReportError(VIR_ERR_INTERNAL_ERROR,
@@ -9234,6 +9306,23 @@ virDomainDiskDefFormat(virBufferPtr buf,
virBufferAsprintf(buf, "/>\n");
}
+ if (def->auth.username) {
+ virBufferAsprintf(buf, " <auth username='%s'>\n",
+ def->auth.username);
+ if (def->auth.secretType == VIR_DOMAIN_DISK_SECRET_TYPE_UUID) {
+ virUUIDFormat(def->auth.secret.uuid, uuidstr);
+ virBufferAsprintf(buf,
+ " <secret type='passphrase' uuid='%s'/>\n",
+ uuidstr);
+ }
+ if (def->auth.secretType == VIR_DOMAIN_DISK_SECRET_TYPE_USAGE) {
+ virBufferAsprintf(buf,
+ " <secret type='passphrase' usage='%s'/>\n",
+ def->auth.secret.usage);
+ }
+ virBufferAsprintf(buf, " </auth>\n");
+ }
+
if (def->src || def->nhosts > 0) {
switch (def->type) {
case VIR_DOMAIN_DISK_TYPE_FILE:
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 2119b5a..0d08040 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -269,6 +269,14 @@ enum virDomainSnapshotState {
VIR_DOMAIN_DISK_SNAPSHOT = VIR_DOMAIN_LAST,
};
+enum virDomainDiskSecretType {
+ VIR_DOMAIN_DISK_SECRET_TYPE_NONE,
+ VIR_DOMAIN_DISK_SECRET_TYPE_UUID,
+ VIR_DOMAIN_DISK_SECRET_TYPE_USAGE,
+
+ VIR_DOMAIN_DISK_SECRET_TYPE_LAST
+};
+
/* Stores the virtual disk configuration */
typedef struct _virDomainDiskDef virDomainDiskDef;
typedef virDomainDiskDef *virDomainDiskDefPtr;
@@ -281,6 +289,14 @@ struct _virDomainDiskDef {
int protocol;
int nhosts;
virDomainDiskHostDefPtr hosts;
+ struct {
+ char *username;
+ int secretType;
+ union {
+ unsigned char uuid[VIR_UUID_BUFLEN];
+ char *usage;
+ } secret;
+ } auth;
char *driverName;
char *driverType;
char *serial;
@@ -1868,6 +1884,7 @@ VIR_ENUM_DECL(virDomainDiskCache)
VIR_ENUM_DECL(virDomainDiskErrorPolicy)
VIR_ENUM_DECL(virDomainDiskProtocol)
VIR_ENUM_DECL(virDomainDiskIo)
+VIR_ENUM_DECL(virDomainDiskSecretType)
VIR_ENUM_DECL(virDomainDiskSnapshot)
VIR_ENUM_DECL(virDomainIoEventFd)
VIR_ENUM_DECL(virDomainVirtioEventIdx)
--
1.7.1
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [RFC PATCH v3 2/4] storage: add auth to virDomainDiskDef
2011-10-20 18:01 ` [RFC PATCH v3 2/4] storage: add auth to virDomainDiskDef Josh Durgin
@ 2011-10-27 8:33 ` Daniel P. Berrange
2011-10-28 18:53 ` [libvirt] " Eric Blake
0 siblings, 1 reply; 27+ messages in thread
From: Daniel P. Berrange @ 2011-10-27 8:33 UTC (permalink / raw)
To: Josh Durgin; +Cc: libvir-list, ceph-devel
On Thu, Oct 20, 2011 at 11:01:25AM -0700, Josh Durgin wrote:
> Add additional fields to let you specify the how to authenticate with a disk.
> The secret to use may be referenced by a usage string or a UUID, i.e.:
>
> <auth username='myuser'>
> <secret type='ceph' usage='secretname'/>
> </auth>
>
> or
>
> <auth username='myuser'>
> <secret type='ceph' uuid='0a81f5b2-8403-7b23-c8d6-21ccc2f80d6f'/>
> </auth>
>
> Signed-off-by: Josh Durgin <josh.durgin@dreamhost.com>
> ---
> docs/schemas/domaincommon.rng | 29 +++++++++++
> src/Makefile.am | 3 +-
> src/conf/domain_conf.c | 105 +++++++++++++++++++++++++++++++++++++---
> src/conf/domain_conf.h | 17 +++++++
> 4 files changed, 145 insertions(+), 9 deletions(-)
>
> diff --git a/src/Makefile.am b/src/Makefile.am
> index 2555f81..7f48981 100644
> --- a/src/Makefile.am
> +++ b/src/Makefile.am
> @@ -128,7 +128,8 @@ DOMAIN_CONF_SOURCES = \
> conf/capabilities.c conf/capabilities.h \
> conf/domain_conf.c conf/domain_conf.h \
> conf/domain_audit.c conf/domain_audit.h \
> - conf/domain_nwfilter.c conf/domain_nwfilter.h
> + conf/domain_nwfilter.c conf/domain_nwfilter.h \
> + conf/secret_conf.c
Unless I'm missing something, I don't think your code changes to
domain_conf.c actually introduce any dependancy on secret_conf.c
You include secret_conf.h, but that is only to get access to one
of the enum values. So there's no dep on the secret_conf.c code
and you can just drop this hunk
>
> DOMAIN_EVENT_SOURCES = \
> conf/domain_event.c conf/domain_event.h
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 5959593..1de3742 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -49,6 +49,7 @@
> #include "virfile.h"
> #include "bitmap.h"
> #include "count-one-bits.h"
> +#include "secret_conf.h"
>
> #define VIR_FROM_THIS VIR_FROM_DOMAIN
>
> @@ -185,6 +186,11 @@ VIR_ENUM_IMPL(virDomainDiskProtocol, VIR_DOMAIN_DISK_PROTOCOL_LAST,
> "rbd",
> "sheepdog")
>
> +VIR_ENUM_IMPL(virDomainDiskSecretType, VIR_DOMAIN_DISK_SECRET_TYPE_LAST,
> + "none",
> + "uuid",
> + "usage")
> +
> VIR_ENUM_IMPL(virDomainDiskIo, VIR_DOMAIN_DISK_IO_LAST,
> "default",
> "native",
> @@ -782,6 +788,9 @@ void virDomainDiskDefFree(virDomainDiskDefPtr def)
> VIR_FREE(def->dst);
> VIR_FREE(def->driverName);
> VIR_FREE(def->driverType);
> + VIR_FREE(def->auth.username);
> + if (def->auth.secretType == VIR_DOMAIN_DISK_SECRET_TYPE_USAGE)
> + VIR_FREE(def->auth.secret.usage);
> virStorageEncryptionFree(def->encryption);
> virDomainDeviceInfoClear(&def->info);
>
> @@ -2298,7 +2307,7 @@ virDomainDiskDefParseXML(virCapsPtr caps,
> unsigned int flags)
> {
> virDomainDiskDefPtr def;
> - xmlNodePtr cur, host;
> + xmlNodePtr cur, child;
> char *type = NULL;
> char *device = NULL;
> char *snapshot = NULL;
> @@ -2319,6 +2328,10 @@ virDomainDiskDefParseXML(virCapsPtr caps,
> char *devaddr = NULL;
> virStorageEncryptionPtr encryption = NULL;
> char *serial = NULL;
> + char *authUsername = NULL;
> + char *authUsage = NULL;
> + char *authUUID = NULL;
> + char *usageType = NULL;
>
> if (VIR_ALLOC(def) < 0) {
> virReportOOMError();
> @@ -2374,10 +2387,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;
> @@ -2386,20 +2399,20 @@ 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;
> + child = child->next;
> }
> break;
> default:
> @@ -2436,6 +2449,58 @@ virDomainDiskDefParseXML(virCapsPtr caps,
> iotag = virXMLPropString(cur, "io");
> ioeventfd = virXMLPropString(cur, "ioeventfd");
> event_idx = virXMLPropString(cur, "event_idx");
> + } else if (xmlStrEqual(cur->name, BAD_CAST "auth")) {
> + authUsername = virXMLPropString(cur, "username");
> + if (authUsername == NULL) {
> + virDomainReportError(VIR_ERR_INTERNAL_ERROR,
> + _("missing username for auth"));
> + goto error;
> + }
> +
> + def->auth.secretType = VIR_DOMAIN_DISK_SECRET_TYPE_NONE;
> + child = cur->children;
> + while (child != NULL) {
> + if (child->type == XML_ELEMENT_NODE &&
> + xmlStrEqual(child->name, BAD_CAST "secret")) {
> + usageType = virXMLPropString(child, "type");
> + if (usageType == NULL) {
> + virDomainReportError(VIR_ERR_XML_ERROR,
> + _("missing type for secret"));
> + goto error;
> + }
> + if (virSecretUsageTypeTypeFromString(usageType) !=
> + VIR_SECRET_USAGE_TYPE_CEPH) {
> + virDomainReportError(VIR_ERR_XML_ERROR,
> + _("invalid secret type %s"),
> + usageType);
> + goto error;
> + }
> +
> + authUUID = virXMLPropString(child, "uuid");
> + authUsage = virXMLPropString(child, "usage");
> +
> + if (authUUID != NULL && authUsage != NULL) {
> + virDomainReportError(VIR_ERR_XML_ERROR,
> + _("only one of uuid and usage can be specfied"));
> + goto error;
> + }
> + if (authUUID != NULL) {
> + def->auth.secretType = VIR_DOMAIN_DISK_SECRET_TYPE_UUID;
> + if (virUUIDParse(authUUID,
> + def->auth.secret.uuid) < 0) {
> + virDomainReportError(VIR_ERR_XML_ERROR,
> + _("malformed uuid %s"),
> + authUUID);
> + goto error;
> + }
> + } else if (authUsage != NULL) {
> + def->auth.secretType = VIR_DOMAIN_DISK_SECRET_TYPE_USAGE;
> + def->auth.secret.usage = authUsage;
> + authUsage = NULL;
> + }
> + }
> + child = child->next;
> + }
> } else if (xmlStrEqual(cur->name, BAD_CAST "readonly")) {
> def->readonly = 1;
> } else if (xmlStrEqual(cur->name, BAD_CAST "shareable")) {
> @@ -2654,6 +2719,8 @@ virDomainDiskDefParseXML(virCapsPtr caps,
> hosts = NULL;
> def->nhosts = nhosts;
> nhosts = 0;
> + def->auth.username = authUsername;
> + authUsername = NULL;
> def->driverName = driverName;
> driverName = NULL;
> def->driverType = driverType;
> @@ -2690,6 +2757,10 @@ cleanup:
> VIR_FREE(hosts);
> VIR_FREE(protocol);
> VIR_FREE(device);
> + VIR_FREE(authUsername);
> + VIR_FREE(usageType);
> + VIR_FREE(authUUID);
> + VIR_FREE(authUsage);
> VIR_FREE(driverType);
> VIR_FREE(driverName);
> VIR_FREE(cachetag);
> @@ -9176,6 +9247,7 @@ virDomainDiskDefFormat(virBufferPtr buf,
> const char *iomode = virDomainDiskIoTypeToString(def->iomode);
> const char *ioeventfd = virDomainIoEventFdTypeToString(def->ioeventfd);
> const char *event_idx = virDomainVirtioEventIdxTypeToString(def->event_idx);
> + char uuidstr[VIR_UUID_STRING_BUFLEN];
>
> if (!type) {
> virDomainReportError(VIR_ERR_INTERNAL_ERROR,
> @@ -9234,6 +9306,23 @@ virDomainDiskDefFormat(virBufferPtr buf,
> virBufferAsprintf(buf, "/>\n");
> }
>
> + if (def->auth.username) {
> + virBufferAsprintf(buf, " <auth username='%s'>\n",
> + def->auth.username);
> + if (def->auth.secretType == VIR_DOMAIN_DISK_SECRET_TYPE_UUID) {
> + virUUIDFormat(def->auth.secret.uuid, uuidstr);
> + virBufferAsprintf(buf,
> + " <secret type='passphrase' uuid='%s'/>\n",
> + uuidstr);
> + }
> + if (def->auth.secretType == VIR_DOMAIN_DISK_SECRET_TYPE_USAGE) {
> + virBufferAsprintf(buf,
> + " <secret type='passphrase' usage='%s'/>\n",
> + def->auth.secret.usage);
> + }
> + virBufferAsprintf(buf, " </auth>\n");
> + }
> +
> if (def->src || def->nhosts > 0) {
> switch (def->type) {
> case VIR_DOMAIN_DISK_TYPE_FILE:
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index 2119b5a..0d08040 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -269,6 +269,14 @@ enum virDomainSnapshotState {
> VIR_DOMAIN_DISK_SNAPSHOT = VIR_DOMAIN_LAST,
> };
>
> +enum virDomainDiskSecretType {
> + VIR_DOMAIN_DISK_SECRET_TYPE_NONE,
> + VIR_DOMAIN_DISK_SECRET_TYPE_UUID,
> + VIR_DOMAIN_DISK_SECRET_TYPE_USAGE,
> +
> + VIR_DOMAIN_DISK_SECRET_TYPE_LAST
> +};
> +
> /* Stores the virtual disk configuration */
> typedef struct _virDomainDiskDef virDomainDiskDef;
> typedef virDomainDiskDef *virDomainDiskDefPtr;
> @@ -281,6 +289,14 @@ struct _virDomainDiskDef {
> int protocol;
> int nhosts;
> virDomainDiskHostDefPtr hosts;
> + struct {
> + char *username;
> + int secretType;
> + union {
> + unsigned char uuid[VIR_UUID_BUFLEN];
> + char *usage;
> + } secret;
> + } auth;
> char *driverName;
> char *driverType;
> char *serial;
> @@ -1868,6 +1884,7 @@ VIR_ENUM_DECL(virDomainDiskCache)
> VIR_ENUM_DECL(virDomainDiskErrorPolicy)
> VIR_ENUM_DECL(virDomainDiskProtocol)
> VIR_ENUM_DECL(virDomainDiskIo)
> +VIR_ENUM_DECL(virDomainDiskSecretType)
> VIR_ENUM_DECL(virDomainDiskSnapshot)
> VIR_ENUM_DECL(virDomainIoEventFd)
> VIR_ENUM_DECL(virDomainVirtioEventIdx)
ACK with the Makefile.am hunk dropped
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] 27+ messages in thread* Re: [libvirt] [RFC PATCH v3 2/4] storage: add auth to virDomainDiskDef
2011-10-27 8:33 ` Daniel P. Berrange
@ 2011-10-28 18:53 ` Eric Blake
2011-10-28 19:15 ` Josh Durgin
0 siblings, 1 reply; 27+ messages in thread
From: Eric Blake @ 2011-10-28 18:53 UTC (permalink / raw)
To: Daniel P. Berrange; +Cc: Josh Durgin, libvir-list, ceph-devel
On 10/27/2011 02:33 AM, Daniel P. Berrange wrote:
> On Thu, Oct 20, 2011 at 11:01:25AM -0700, Josh Durgin wrote:
>> Add additional fields to let you specify the how to authenticate with a disk.
>> The secret to use may be referenced by a usage string or a UUID, i.e.:
>>
>> <auth username='myuser'>
>> <secret type='ceph' usage='secretname'/>
>> </auth>
>>
>> or
>>
>> <auth username='myuser'>
>> <secret type='ceph' uuid='0a81f5b2-8403-7b23-c8d6-21ccc2f80d6f'/>
>> </auth>
>>
>> +++ b/src/Makefile.am
>> @@ -128,7 +128,8 @@ DOMAIN_CONF_SOURCES = \
>> conf/capabilities.c conf/capabilities.h \
>> conf/domain_conf.c conf/domain_conf.h \
>> conf/domain_audit.c conf/domain_audit.h \
>> - conf/domain_nwfilter.c conf/domain_nwfilter.h
>> + conf/domain_nwfilter.c conf/domain_nwfilter.h \
>> + conf/secret_conf.c
>
>
> Unless I'm missing something, I don't think your code changes to
> domain_conf.c actually introduce any dependancy on secret_conf.c
> You include secret_conf.h, but that is only to get access to one
> of the enum values. So there's no dep on the secret_conf.c code
> and you can just drop this hunk
Actually, the linker now wants to pull in
virSecretUsageTypeTypeFromString (yuck; why do we have that doubled
"Type" in the name?), so that means more drivers have to add a link
library, to prevent problems like this:
libvirt_lxc-domain_conf.o: In function `virDomainDiskDefParseXML':
/home/remote/eblake/libvirt/src/conf/domain_conf.c:2479: undefined
reference to `virSecretUsageTypeTypeFromString'
>> + </attribute>
>> + <attribute name="usage">
>> + <ref name="genericName"/>
This says usage='name' uses a genericName, but in secret.rng, you said
element <name> could use arbitrary text - that is, we have a discrepancy
where the secret could have an arbitrary name which validates for
secret.rng but fails to validate for <auth> as part of domain.rng. You
probably ought to do a followup patch that consolidates the two .rng
files to use the same definition for what you will accept as a valid
Ceph secret name.
>>
>> + if (def->auth.username) {
>> + virBufferAsprintf(buf, "<auth username='%s'>\n",
>> + def->auth.username);
>> + if (def->auth.secretType == VIR_DOMAIN_DISK_SECRET_TYPE_UUID) {
>> + virUUIDFormat(def->auth.secret.uuid, uuidstr);
>> + virBufferAsprintf(buf,
>> + "<secret type='passphrase' uuid='%s'/>\n",
This disagrees with your type='ceph' in the commit message (twice). You
would have caught this had you added a test that does round-trip from
XML in and back out somewhere in the series. Could you please do that
as a followup patch?
>> + uuidstr);
>> + }
>> + if (def->auth.secretType == VIR_DOMAIN_DISK_SECRET_TYPE_USAGE) {
>> + virBufferAsprintf(buf,
This must use virBufferEscapeString, since the user's "usage" string may
have arbitrary text.
>> + "<secret type='passphrase' usage='%s'/>\n",
>> + def->auth.secret.usage);
>> + }
>> + virBufferAsprintf(buf, "</auth>\n");
AddLit is more efficient than Asprintf for a constant string.
>> +enum virDomainDiskSecretType {
>> + VIR_DOMAIN_DISK_SECRET_TYPE_NONE,
>> + VIR_DOMAIN_DISK_SECRET_TYPE_UUID,
>> + VIR_DOMAIN_DISK_SECRET_TYPE_USAGE,
>> +
>> + VIR_DOMAIN_DISK_SECRET_TYPE_LAST
>> +};
>> +
>> /* Stores the virtual disk configuration */
>> typedef struct _virDomainDiskDef virDomainDiskDef;
>> typedef virDomainDiskDef *virDomainDiskDefPtr;
>> @@ -281,6 +289,14 @@ struct _virDomainDiskDef {
>> int protocol;
>> int nhosts;
>> virDomainDiskHostDefPtr hosts;
>> + struct {
>> + char *username;
>> + int secretType;
I like to add a comment stating which values are expected in this field
(here, enum virDomainDiskSecretType).
>
> ACK with the Makefile.am hunk dropped
Also missing documentation. Here's what I had to squash in for that,
before pushing. Also, I added Josh to AUTHORS (shoot, I also realized
that I botched Josh's email in 1/4 when hand-applying everything, due to
battling the lost emails, sorry about that).
diff --git i/docs/formatdomain.html.in w/docs/formatdomain.html.in
index fcffb25..f31b775 100644
--- i/docs/formatdomain.html.in
+++ w/docs/formatdomain.html.in
@@ -913,6 +913,16 @@
<transient/>
<address type='drive' controller='0' bus='1' unit='0'/>
</disk>
+ <disk type='network'>
+ <driver name="qemu" type="raw"/>
+ <source protocol="rbd" name="image_name2">
+ <host name="hostname" port="7000"/>
+ </source>
+ <target dev="hdd" bus="ide"/>
+ <auth username='myuser'>
+ <secret type='ceph' usage='mypassid'/>
+ </auth>
+ </disk>
<disk type='block' device='cdrom'>
<driver name='qemu' type='raw'/>
<target def='hdc' bus='ide'/>
@@ -1160,7 +1170,24 @@
"drive" controller, additional attributes
<code>controller</code>, <code>bus</code>,
and <code>unit</code> are available, each defaulting to 0.
-
+ </dd>
+ <dt><code>auth</code></dt>
+ <dd>If present, the <code>auth</code> element provides the
+ authentication credentials needed to access the source. It
+ includes a mandatory attribute <code>username</code>, which
+ identifies the username to use during authentication, as well
+ as a sub-element <code>secret</code> with mandatory
+ attribute <code>type</code>, to tie back to
+ a <a href="formatsecret.html">libvirt secret object</a> that
+ holds the actual password or other credentials (the domain XML
+ intentionally does not expose the password, only the reference
+ to the object that does manage the password). For now, the
+ only known secret <code>type</code> is "ceph", for Ceph RBD
+ network sources, and requires either an
+ attribute <code>uuid</code> with the UUID of the Ceph secret
+ object, or an attribute <code>usage</code> with the name
+ associated with the Ceph secret
+ object. <span class="since">libvirt 0.9.7</span>
</dd>
</dl>
diff --git i/src/Makefile.am w/src/Makefile.am
index 5b3a66f..995afae 100644
--- i/src/Makefile.am
+++ w/src/Makefile.am
@@ -128,8 +128,7 @@ DOMAIN_CONF_SOURCES = \
conf/capabilities.c conf/capabilities.h \
conf/domain_conf.c conf/domain_conf.h \
conf/domain_audit.c conf/domain_audit.h \
- conf/domain_nwfilter.c conf/domain_nwfilter.h \
- conf/secret_conf.c conf/secret_conf.h
+ conf/domain_nwfilter.c conf/domain_nwfilter.h
DOMAIN_EVENT_SOURCES = \
conf/domain_event.c conf/domain_event.h
@@ -1476,6 +1475,7 @@ libvirt_lxc_SOURCES = \
$(NODE_INFO_SOURCES) \
$(ENCRYPTION_CONF_SOURCES) \
$(DOMAIN_CONF_SOURCES) \
+ $(SECRET_CONF_SOURCES) \
$(CPU_CONF_SOURCES) \
$(NWFILTER_PARAM_CONF_SOURCES)
libvirt_lxc_LDFLAGS = $(WARN_CFLAGS) $(AM_LDFLAGS)
diff --git i/src/libvirt_private.syms w/src/libvirt_private.syms
index 987c512..288911a 100644
--- i/src/libvirt_private.syms
+++ w/src/libvirt_private.syms
@@ -930,6 +930,8 @@ virSecretDefFormat;
virSecretDefFree;
virSecretDefParseFile;
virSecretDefParseString;
+virSecretUsageTypeTypeFromString;
+virSecretUsageTypeTypeToString;
# security_driver.h
--
Eric Blake eblake@redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [libvirt] [RFC PATCH v3 2/4] storage: add auth to virDomainDiskDef
2011-10-28 18:53 ` [libvirt] " Eric Blake
@ 2011-10-28 19:15 ` Josh Durgin
2011-10-28 21:19 ` [PATCH 1/1] Use a common xml type for ceph secret usage Josh Durgin
0 siblings, 1 reply; 27+ messages in thread
From: Josh Durgin @ 2011-10-28 19:15 UTC (permalink / raw)
To: Eric Blake; +Cc: Daniel P. Berrange, libvir-list, ceph-devel
On 10/28/2011 11:53 AM, Eric Blake wrote:
> On 10/27/2011 02:33 AM, Daniel P. Berrange wrote:
>> On Thu, Oct 20, 2011 at 11:01:25AM -0700, Josh Durgin wrote:
>>> Add additional fields to let you specify the how to authenticate with
>>> a disk.
>>> The secret to use may be referenced by a usage string or a UUID, i.e.:
>>>
>>> <auth username='myuser'>
>>> <secret type='ceph' usage='secretname'/>
>>> </auth>
>>>
>>> or
>>>
>>> <auth username='myuser'>
>>> <secret type='ceph' uuid='0a81f5b2-8403-7b23-c8d6-21ccc2f80d6f'/>
>>> </auth>
>>>
>
>>> +++ b/src/Makefile.am
>>> @@ -128,7 +128,8 @@ DOMAIN_CONF_SOURCES = \
>>> conf/capabilities.c conf/capabilities.h \
>>> conf/domain_conf.c conf/domain_conf.h \
>>> conf/domain_audit.c conf/domain_audit.h \
>>> - conf/domain_nwfilter.c conf/domain_nwfilter.h
>>> + conf/domain_nwfilter.c conf/domain_nwfilter.h \
>>> + conf/secret_conf.c
>>
>>
>> Unless I'm missing something, I don't think your code changes to
>> domain_conf.c actually introduce any dependancy on secret_conf.c
>> You include secret_conf.h, but that is only to get access to one
>> of the enum values. So there's no dep on the secret_conf.c code
>> and you can just drop this hunk
>
> Actually, the linker now wants to pull in
> virSecretUsageTypeTypeFromString (yuck; why do we have that doubled
> "Type" in the name?), so that means more drivers have to add a link
> library, to prevent problems like this:
>
> libvirt_lxc-domain_conf.o: In function `virDomainDiskDefParseXML':
> /home/remote/eblake/libvirt/src/conf/domain_conf.c:2479: undefined
> reference to `virSecretUsageTypeTypeFromString'
>
>>> + </attribute>
>>> + <attribute name="usage">
>>> + <ref name="genericName"/>
>
> This says usage='name' uses a genericName, but in secret.rng, you said
> element <name> could use arbitrary text - that is, we have a discrepancy
> where the secret could have an arbitrary name which validates for
> secret.rng but fails to validate for <auth> as part of domain.rng. You
> probably ought to do a followup patch that consolidates the two .rng
> files to use the same definition for what you will accept as a valid
> Ceph secret name.
Yeah, I'll fix that.
>>>
>>> + if (def->auth.username) {
>>> + virBufferAsprintf(buf, "<auth username='%s'>\n",
>>> + def->auth.username);
>>> + if (def->auth.secretType == VIR_DOMAIN_DISK_SECRET_TYPE_UUID) {
>>> + virUUIDFormat(def->auth.secret.uuid, uuidstr);
>>> + virBufferAsprintf(buf,
>>> + "<secret type='passphrase' uuid='%s'/>\n",
>
> This disagrees with your type='ceph' in the commit message (twice). You
> would have caught this had you added a test that does round-trip from
> XML in and back out somewhere in the series. Could you please do that as
> a followup patch?
Oops, sorry about that. The reason I didn't include a test going from
commandline to secret is that we're going to be passing the secret
through the qemu monitor so it won't be exposed on the command line.
>>> + uuidstr);
>>> + }
>>> + if (def->auth.secretType == VIR_DOMAIN_DISK_SECRET_TYPE_USAGE) {
>>> + virBufferAsprintf(buf,
>
> This must use virBufferEscapeString, since the user's "usage" string may
> have arbitrary text.
>
>>> + "<secret type='passphrase' usage='%s'/>\n",
>>> + def->auth.secret.usage);
>>> + }
>>> + virBufferAsprintf(buf, "</auth>\n");
>
> AddLit is more efficient than Asprintf for a constant string.
>
>>> +enum virDomainDiskSecretType {
>>> + VIR_DOMAIN_DISK_SECRET_TYPE_NONE,
>>> + VIR_DOMAIN_DISK_SECRET_TYPE_UUID,
>>> + VIR_DOMAIN_DISK_SECRET_TYPE_USAGE,
>>> +
>>> + VIR_DOMAIN_DISK_SECRET_TYPE_LAST
>>> +};
>>> +
>>> /* Stores the virtual disk configuration */
>>> typedef struct _virDomainDiskDef virDomainDiskDef;
>>> typedef virDomainDiskDef *virDomainDiskDefPtr;
>>> @@ -281,6 +289,14 @@ struct _virDomainDiskDef {
>>> int protocol;
>>> int nhosts;
>>> virDomainDiskHostDefPtr hosts;
>>> + struct {
>>> + char *username;
>>> + int secretType;
>
> I like to add a comment stating which values are expected in this field
> (here, enum virDomainDiskSecretType).
>
>>
>> ACK with the Makefile.am hunk dropped
>
> Also missing documentation. Here's what I had to squash in for that,
> before pushing. Also, I added Josh to AUTHORS (shoot, I also realized
> that I botched Josh's email in 1/4 when hand-applying everything, due to
> battling the lost emails, sorry about that).
>
> diff --git i/docs/formatdomain.html.in w/docs/formatdomain.html.in
> index fcffb25..f31b775 100644
> --- i/docs/formatdomain.html.in
> +++ w/docs/formatdomain.html.in
> @@ -913,6 +913,16 @@
> <transient/>
> <address type='drive' controller='0' bus='1' unit='0'/>
> </disk>
> + <disk type='network'>
> + <driver name="qemu" type="raw"/>
> + <source protocol="rbd" name="image_name2">
> + <host name="hostname" port="7000"/>
> + </source>
> + <target dev="hdd" bus="ide"/>
> + <auth username='myuser'>
> + <secret type='ceph' usage='mypassid'/>
> + </auth>
> + </disk>
> <disk type='block' device='cdrom'>
> <driver name='qemu' type='raw'/>
> <target def='hdc' bus='ide'/>
> @@ -1160,7 +1170,24 @@
> "drive" controller, additional attributes
> <code>controller</code>, <code>bus</code>,
> and <code>unit</code> are available, each defaulting to 0.
> -
> + </dd>
> + <dt><code>auth</code></dt>
> + <dd>If present, the <code>auth</code> element provides the
> + authentication credentials needed to access the source. It
> + includes a mandatory attribute <code>username</code>, which
> + identifies the username to use during authentication, as well
> + as a sub-element <code>secret</code> with mandatory
> + attribute <code>type</code>, to tie back to
> + a <a href="formatsecret.html">libvirt secret object</a> that
> + holds the actual password or other credentials (the domain XML
> + intentionally does not expose the password, only the reference
> + to the object that does manage the password). For now, the
> + only known secret <code>type</code> is "ceph", for Ceph RBD
> + network sources, and requires either an
> + attribute <code>uuid</code> with the UUID of the Ceph secret
> + object, or an attribute <code>usage</code> with the name
> + associated with the Ceph secret
> + object. <span class="since">libvirt 0.9.7</span>
> </dd>
> </dl>
>
> diff --git i/src/Makefile.am w/src/Makefile.am
> index 5b3a66f..995afae 100644
> --- i/src/Makefile.am
> +++ w/src/Makefile.am
> @@ -128,8 +128,7 @@ DOMAIN_CONF_SOURCES = \
> conf/capabilities.c conf/capabilities.h \
> conf/domain_conf.c conf/domain_conf.h \
> conf/domain_audit.c conf/domain_audit.h \
> - conf/domain_nwfilter.c conf/domain_nwfilter.h \
> - conf/secret_conf.c conf/secret_conf.h
> + conf/domain_nwfilter.c conf/domain_nwfilter.h
>
> DOMAIN_EVENT_SOURCES = \
> conf/domain_event.c conf/domain_event.h
> @@ -1476,6 +1475,7 @@ libvirt_lxc_SOURCES = \
> $(NODE_INFO_SOURCES) \
> $(ENCRYPTION_CONF_SOURCES) \
> $(DOMAIN_CONF_SOURCES) \
> + $(SECRET_CONF_SOURCES) \
> $(CPU_CONF_SOURCES) \
> $(NWFILTER_PARAM_CONF_SOURCES)
> libvirt_lxc_LDFLAGS = $(WARN_CFLAGS) $(AM_LDFLAGS)
>
> diff --git i/src/libvirt_private.syms w/src/libvirt_private.syms
> index 987c512..288911a 100644
> --- i/src/libvirt_private.syms
> +++ w/src/libvirt_private.syms
> @@ -930,6 +930,8 @@ virSecretDefFormat;
> virSecretDefFree;
> virSecretDefParseFile;
> virSecretDefParseString;
> +virSecretUsageTypeTypeFromString;
> +virSecretUsageTypeTypeToString;
>
>
> # security_driver.h
>
^ permalink raw reply [flat|nested] 27+ messages in thread* [PATCH 1/1] Use a common xml type for ceph secret usage.
2011-10-28 19:15 ` Josh Durgin
@ 2011-10-28 21:19 ` Josh Durgin
2011-10-28 22:03 ` Eric Blake
0 siblings, 1 reply; 27+ messages in thread
From: Josh Durgin @ 2011-10-28 21:19 UTC (permalink / raw)
To: libvir-list; +Cc: ceph-devel, eblake
The types used in domaincommon.rng and secret.rng should be the same.
Signed-off-by: Josh Durgin <josh.durgin@dreamhost.com>
---
docs/schemas/domaincommon.rng | 11 ++++++++---
docs/schemas/secret.rng | 4 +++-
2 files changed, 11 insertions(+), 4 deletions(-)
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index 3477351..d053489 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -2558,13 +2558,13 @@
<attribute name='uuid'>
<ref name="UUID"/>
</attribute>
- <attribute name="usage">
- <ref name="genericName"/>
+ <attribute name='usage'>
+ <ref name='usageName'/>
</attribute>
</choice>
</element>
</define>
-
+
<!--
Optional hypervisor extensions in their own namespace:
QEmu
@@ -2675,6 +2675,11 @@
<param name="pattern">(([0-2]?[0-9]?[0-9]\.){3}[0-2]?[0-9]?[0-9])|(([0-9a-fA-F]+|:)+[0-9a-fA-F]+)|([a-zA-Z0-9_\.\+\-]*)</param>
</data>
</define>
+ <define name="usageName">
+ <data type="string">
+ <param name="pattern">[a-zA-Z0-9_\.\+\-]+</param>
+ </data>
+ </define>
<define name="usbId">
<data type="string">
<param name="pattern">(0x)?[0-9a-fA-F]{1,4}</param>
diff --git a/docs/schemas/secret.rng b/docs/schemas/secret.rng
index 8e7714b..3abd3c7 100644
--- a/docs/schemas/secret.rng
+++ b/docs/schemas/secret.rng
@@ -4,6 +4,8 @@
<ref name='secret'/>
</start>
+ <include href='domaincommon.rng'/>
+
<define name='secret'>
<element name='secret'>
<optional>
@@ -60,7 +62,7 @@
<value>ceph</value>
</attribute>
<element name='name'>
- <text/>
+ <ref name='usageName'/>
</element>
</define>
--
1.7.1
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH 1/1] Use a common xml type for ceph secret usage.
2011-10-28 21:19 ` [PATCH 1/1] Use a common xml type for ceph secret usage Josh Durgin
@ 2011-10-28 22:03 ` Eric Blake
0 siblings, 0 replies; 27+ messages in thread
From: Eric Blake @ 2011-10-28 22:03 UTC (permalink / raw)
To: Josh Durgin; +Cc: libvir-list, ceph-devel
On 10/28/2011 03:19 PM, Josh Durgin wrote:
> The types used in domaincommon.rng and secret.rng should be the same.
>
> Signed-off-by: Josh Durgin<josh.durgin@dreamhost.com>
> ---
> docs/schemas/domaincommon.rng | 11 ++++++++---
> docs/schemas/secret.rng | 4 +++-
> 2 files changed, 11 insertions(+), 4 deletions(-)
>
> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
> index 3477351..d053489 100644
> --- a/docs/schemas/domaincommon.rng
> +++ b/docs/schemas/domaincommon.rng
> @@ -2558,13 +2558,13 @@
> <attribute name='uuid'>
> <ref name="UUID"/>
> </attribute>
> -<attribute name="usage">
> -<ref name="genericName"/>
> +<attribute name='usage'>
> +<ref name='usageName'/>
genericName is probably fine instead of inventing usageName; but I'll
graduate it out of domaincommon.rng into basictypes.rng.
> </attribute>
> </choice>
> </element>
> </define>
> -
> +
Spurious whitespace change.
> +++ b/docs/schemas/secret.rng
> @@ -4,6 +4,8 @@
> <ref name='secret'/>
> </start>
>
> +<include href='domaincommon.rng'/>
domaincommon.rng is rather heavyweight; basictypes.rng is better.
> +
> <define name='secret'>
> <element name='secret'>
> <optional>
> @@ -60,7 +62,7 @@
> <value>ceph</value>
> </attribute>
> <element name='name'>
> -<text/>
> +<ref name='usageName'/>
> </element>
> </define>
At any rate, once we include common types, we can stop repeating the
UUID definition too.
Not quite like your original patch, but I'll go ahead and squash this
in, then push in your name since it matches up with your intent.
diff --git i/docs/schemas/basictypes.rng w/docs/schemas/basictypes.rng
index b3267f5..3b4b952 100644
--- i/docs/schemas/basictypes.rng
+++ w/docs/schemas/basictypes.rng
@@ -97,6 +97,12 @@
</choice>
</define>
+ <define name="genericName">
+ <data type="string">
+ <param name="pattern">[a-zA-Z0-9_\+\-]+</param>
+ </data>
+ </define>
+
<define name="dnsName">
<data type="string">
<param name="pattern">[a-zA-Z0-9\.\-]+</param>
diff --git i/docs/schemas/domaincommon.rng w/docs/schemas/domaincommon.rng
index d053489..b6f858e 100644
--- i/docs/schemas/domaincommon.rng
+++ w/docs/schemas/domaincommon.rng
@@ -2559,12 +2559,12 @@
<ref name="UUID"/>
</attribute>
<attribute name='usage'>
- <ref name='usageName'/>
+ <ref name='genericName'/>
</attribute>
</choice>
</element>
</define>
-
+
<!--
Optional hypervisor extensions in their own namespace:
QEmu
@@ -2660,11 +2660,6 @@
<param name="pattern">[A-Za-z0-9_\.\+\-]+</param>
</data>
</define>
- <define name="genericName">
- <data type="string">
- <param name="pattern">[a-zA-Z0-9_\+\-]+</param>
- </data>
- </define>
<define name="bridgeMode">
<data type="string">
<param name="pattern">(vepa|bridge|private|passthrough)</param>
@@ -2675,11 +2670,6 @@
<param
name="pattern">(([0-2]?[0-9]?[0-9]\.){3}[0-2]?[0-9]?[0-9])|(([0-9a-fA-F]+|:)+[0-9a-fA-F]+)|([a-zA-Z0-9_\.\+\-]*)</param>
</data>
</define>
- <define name="usageName">
- <data type="string">
- <param name="pattern">[a-zA-Z0-9_\.\+\-]+</param>
- </data>
- </define>
<define name="usbId">
<data type="string">
<param name="pattern">(0x)?[0-9a-fA-F]{1,4}</param>
diff --git i/docs/schemas/secret.rng w/docs/schemas/secret.rng
index 3abd3c7..e49bd5a 100644
--- i/docs/schemas/secret.rng
+++ w/docs/schemas/secret.rng
@@ -1,10 +1,11 @@
+<?xml version="1.0"?>
<!-- A Relax NG schema for the libvirt secret properties XML format -->
<grammar xmlns="http://relaxng.org/ns/structure/1.0">
<start>
<ref name='secret'/>
</start>
- <include href='domaincommon.rng'/>
+ <include href='basictypes.rng'/>
<define name='secret'>
<element name='secret'>
@@ -62,25 +63,8 @@
<value>ceph</value>
</attribute>
<element name='name'>
- <ref name='usageName'/>
+ <ref name='genericName'/>
</element>
</define>
- <define name="UUID">
- <choice>
- <data type="string">
- <param name="pattern">[a-fA-F0-9]{32}</param>
- </data>
- <data type="string">
- <param
name="pattern">[a-fA-F0-9]{8}\-([a-fA-F0-9]{4}\-){3}[a-fA-F0-9]{12}</param>
- </data>
- </choice>
- </define>
-
- <define name="absFilePath">
- <data type="string">
- <param name="pattern">/[a-zA-Z0-9_\.\+\-&/%]+</param>
- </data>
- </define>
-
</grammar>
--
Eric Blake eblake@redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [RFC PATCH v3 3/4] qemu: pass virConnectPtr into Domain{Attach,Detach}*
2011-10-20 18:01 [RFC PATCH v3 0/4] Improve Ceph Qemu+RBD support Josh Durgin
2011-10-20 18:01 ` [RFC PATCH v3 1/4] secret: add Ceph secret type Josh Durgin
2011-10-20 18:01 ` [RFC PATCH v3 2/4] storage: add auth to virDomainDiskDef Josh Durgin
@ 2011-10-20 18:01 ` Josh Durgin
2011-10-27 8:33 ` Daniel P. Berrange
2011-10-20 18:01 ` [RFC PATCH v3 4/4] qemu/rbd: improve rbd device specification Josh Durgin
2011-10-27 5:19 ` [RFC PATCH v3 0/4] Improve Ceph Qemu+RBD support Sage Weil
4 siblings, 1 reply; 27+ messages in thread
From: Josh Durgin @ 2011-10-20 18:01 UTC (permalink / raw)
To: libvir-list; +Cc: ceph-devel, berrange
From: Sage Weil <sage@newdream.net>
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 30c0be6..4dfce88 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -1491,7 +1491,8 @@ qemuSafeSerialParamValue(const char *value)
char *
-qemuBuildDriveStr(virDomainDiskDefPtr disk,
+qemuBuildDriveStr(virConnectPtr conn,
+ virDomainDiskDefPtr disk,
bool bootable,
virBitmapPtr qemuCaps)
{
@@ -3908,7 +3909,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 00e58a2..cd0c850 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 84ef4dd..4b707d1 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -5038,7 +5038,8 @@ qemudDomainUndefine(virDomainPtr dom)
}
static int
-qemuDomainAttachDeviceDiskLive(struct qemud_driver *driver,
+qemuDomainAttachDeviceDiskLive(virConnectPtr conn,
+ struct qemud_driver *driver,
virDomainObjPtr vm,
virDomainDeviceDefPtr dev)
{
@@ -5070,12 +5071,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."),
@@ -5131,7 +5132,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;
@@ -5692,8 +5693,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);
}
@@ -5712,8 +5712,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 bfa524b..98f9850 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -185,7 +185,8 @@ cleanup:
}
-int qemuDomainAttachPciDiskDevice(struct qemud_driver *driver,
+int qemuDomainAttachPciDiskDevice(virConnectPtr conn,
+ struct qemud_driver *driver,
virDomainObjPtr vm,
virDomainDiskDefPtr disk)
{
@@ -221,7 +222,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)))
@@ -414,7 +415,8 @@ qemuDomainFindOrCreateSCSIDiskController(struct qemud_driver *driver,
}
-int qemuDomainAttachSCSIDisk(struct qemud_driver *driver,
+int qemuDomainAttachSCSIDisk(virConnectPtr conn,
+ struct qemud_driver *driver,
virDomainObjPtr vm,
virDomainDiskDefPtr disk)
{
@@ -458,7 +460,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++) {
@@ -537,7 +539,8 @@ error:
}
-int qemuDomainAttachUsbMassstorageDevice(struct qemud_driver *driver,
+int qemuDomainAttachUsbMassstorageDevice(virConnectPtr conn,
+ struct qemud_driver *driver,
virDomainObjPtr vm,
virDomainDiskDefPtr disk)
{
@@ -574,7 +577,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 aaaed88..8353f14 100644
--- a/src/qemu/qemu_hotplug.h
+++ b/src/qemu/qemu_hotplug.h
@@ -33,16 +33,19 @@ int qemuDomainChangeEjectableMedia(struct qemud_driver *driver,
bool force);
int qemuDomainCheckEjectableMedia(struct qemud_driver *driver,
virDomainObjPtr vm);
-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.1
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [RFC PATCH v3 3/4] qemu: pass virConnectPtr into Domain{Attach,Detach}*
2011-10-20 18:01 ` [RFC PATCH v3 3/4] qemu: pass virConnectPtr into Domain{Attach,Detach}* Josh Durgin
@ 2011-10-27 8:33 ` Daniel P. Berrange
2011-10-31 19:14 ` [libvirt] [RFC PATCH v3 3/4] qemu: pass virConnectPtr into Domain{Attach, Detach}* Eric Blake
0 siblings, 1 reply; 27+ messages in thread
From: Daniel P. Berrange @ 2011-10-27 8:33 UTC (permalink / raw)
To: Josh Durgin; +Cc: libvir-list, ceph-devel
On Thu, Oct 20, 2011 at 11:01:26AM -0700, Josh Durgin wrote:
> From: Sage Weil <sage@newdream.net>
>
> 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(-)
Trivial, ACK
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] 27+ messages in thread* Re: [libvirt] [RFC PATCH v3 3/4] qemu: pass virConnectPtr into Domain{Attach, Detach}*
2011-10-27 8:33 ` Daniel P. Berrange
@ 2011-10-31 19:14 ` Eric Blake
0 siblings, 0 replies; 27+ messages in thread
From: Eric Blake @ 2011-10-31 19:14 UTC (permalink / raw)
To: Daniel P. Berrange; +Cc: Josh Durgin, libvir-list, ceph-devel
On 10/27/2011 02:33 AM, Daniel P. Berrange wrote:
> On Thu, Oct 20, 2011 at 11:01:26AM -0700, Josh Durgin wrote:
>> From: Sage Weil<sage@newdream.net>
>>
>> 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(-)
>
> Trivial, ACK
Now pushed.
--
Eric Blake eblake@redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
^ permalink raw reply [flat|nested] 27+ messages in thread
* [RFC PATCH v3 4/4] qemu/rbd: improve rbd device specification
2011-10-20 18:01 [RFC PATCH v3 0/4] Improve Ceph Qemu+RBD support Josh Durgin
` (2 preceding siblings ...)
2011-10-20 18:01 ` [RFC PATCH v3 3/4] qemu: pass virConnectPtr into Domain{Attach,Detach}* Josh Durgin
@ 2011-10-20 18:01 ` Josh Durgin
2011-10-27 8:38 ` Daniel P. Berrange
2011-10-27 5:19 ` [RFC PATCH v3 0/4] Improve Ceph Qemu+RBD support Sage Weil
4 siblings, 1 reply; 27+ messages in thread
From: Josh Durgin @ 2011-10-20 18:01 UTC (permalink / raw)
To: libvir-list; +Cc: ceph-devel, berrange
From: Sage Weil <sage@newdream.net>
This improves the support for qemu rbd devices by adding support for a few
key features (e.g., authentication) and cleaning up the way in which
rbd configuration options are passed to qemu.
And <auth> member of the disk source xml specifies how librbd should
authenticate. The username attribute is the Ceph/RBD user to authenticate as.
The usage or uuid attributes specify which secret to use. Usage is an
arbitrary identifier local to libvirt.
The old RBD support relied on setting an environment variable to
communicate information to qemu/librbd. Instead, pass those options
explicitly to qemu. Update the qemu argument parsing and tests
accordingly.
Signed-off-by: Sage Weil <sage@newdream.net>
Signed-off-by: Josh Durgin <josh.durgin@dreamhost.com>
---
src/qemu/qemu_command.c | 284 ++++++++++++--------
.../qemuxml2argv-disk-drive-network-rbd-auth.args | 6 +
.../qemuxml2argv-disk-drive-network-rbd-auth.xml | 37 +++
.../qemuxml2argv-disk-drive-network-rbd.args | 6 +-
tests/qemuxml2argvtest.c | 52 ++++
5 files changed, 268 insertions(+), 117 deletions(-)
create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-auth.args
create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-auth.xml
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 4dfce88..b2c0eee 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -38,6 +38,7 @@
#include "domain_audit.h"
#include "domain_conf.h"
#include "network/bridge_driver.h"
+#include "base64.h"
#include <sys/utsname.h>
#include <sys/stat.h>
@@ -1489,6 +1490,159 @@ qemuSafeSerialParamValue(const char *value)
return 0;
}
+static int buildRBDString(virConnectPtr conn,
+ virDomainDiskDefPtr disk,
+ virBufferPtr opt)
+{
+ int i;
+ virSecretPtr sec = NULL;
+ char *secret = NULL;
+ size_t secret_size;
+
+ virBufferAsprintf(opt, "rbd:%s", disk->src);
+ if (disk->auth.username) {
+ virBufferEscape(opt, ":", ":id=%s", disk->auth.username);
+ /* look up secret */
+ switch (disk->auth.secretType) {
+ case VIR_DOMAIN_DISK_SECRET_TYPE_UUID:
+ sec = virSecretLookupByUUID(conn,
+ disk->auth.secret.uuid);
+ break;
+ case VIR_DOMAIN_DISK_SECRET_TYPE_USAGE:
+ sec = virSecretLookupByUsage(conn,
+ VIR_SECRET_USAGE_TYPE_CEPH,
+ disk->auth.secret.usage);
+ break;
+ }
+
+ if (sec) {
+ char *base64;
+
+ secret = (char *)conn->secretDriver->getValue(sec, &secret_size, 0,
+ VIR_SECRET_GET_VALUE_INTERNAL_CALL);
+ if (secret == NULL) {
+ qemuReportError(VIR_ERR_INTERNAL_ERROR,
+ _("could not get the value of the secret for username %s"),
+ disk->auth.username);
+ return -1;
+ }
+ /* qemu/librbd wants it base64 encoded */
+ base64_encode_alloc(secret, secret_size, &base64);
+ virBufferEscape(opt, ":", ":key=%s:auth_supported=cephx\\;none",
+ base64);
+ VIR_FREE(base64);
+ VIR_FREE(secret);
+ virUnrefSecret(sec);
+ } else {
+ qemuReportError(VIR_ERR_INTERNAL_ERROR,
+ _("rbd username '%s' specified but secret not found"),
+ disk->auth.username);
+ return -1;
+ }
+ }
+
+ if (disk->nhosts > 0) {
+ virBufferStrcat(opt, ":mon_host=", NULL);
+ for (i = 0; i < disk->nhosts; ++i) {
+ if (i) {
+ virBufferStrcat(opt, "\\;", NULL);
+ }
+ if (disk->hosts[i].port) {
+ virBufferAsprintf(opt, "%s\\:%s",
+ disk->hosts[i].name,
+ disk->hosts[i].port);
+ } else {
+ virBufferAsprintf(opt, "%s", disk->hosts[i].name);
+ }
+ }
+ }
+
+ return 0;
+}
+
+static int addRBDHost(virDomainDiskDefPtr disk, char *hostport)
+{
+ 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->auth.username = strdup(p + strlen("id="));
+ }
+ if (STRPREFIX(p, "mon_host=")) {
+ char *h, *sep;
+
+ h = p + strlen("mon_host=");
+ while (h < e) {
+ for (sep = h; sep < e; ++sep) {
+ if (*sep == '\\' && (sep[1] == ',' ||
+ sep[1] == ';' ||
+ sep[1] == ' ')) {
+ *sep = '\0';
+ sep += 2;
+ break;
+ }
+ }
+ addRBDHost(disk, h);
+ h = sep;
+ }
+ }
+
+ p = next;
+ }
+ return 0;
+}
char *
qemuBuildDriveStr(virConnectPtr conn,
@@ -1608,8 +1762,10 @@ qemuBuildDriveStr(virConnectPtr conn,
disk->hosts->name, disk->hosts->port);
break;
case VIR_DOMAIN_DISK_PROTOCOL_RBD:
- /* TODO: set monitor hostnames */
- virBufferAsprintf(&opt, "file=rbd:%s,", disk->src);
+ virBufferStrcat(&opt, "file=", NULL);
+ if (buildRBDString(conn, disk, &opt) < 0)
+ goto error;
+ virBufferStrcat(&opt, ",", NULL);
break;
case VIR_DOMAIN_DISK_PROTOCOL_SHEEPDOG:
if (disk->nhosts == 0)
@@ -3278,8 +3434,6 @@ qemuBuildCommandLine(virConnectPtr conn,
int last_good_net = -1;
bool hasHwVirt = false;
virCommandPtr cmd;
- bool has_rbd_hosts = false;
- virBuffer rbd_hosts = VIR_BUFFER_INITIALIZER;
bool emitBootindex = false;
int usbcontroller = 0;
bool usblegacy = false;
@@ -3861,7 +4015,6 @@ qemuBuildCommandLine(virConnectPtr conn,
virDomainDiskDefPtr disk = def->disks[i];
int withDeviceArg = 0;
bool deviceFlagMasked = false;
- int j;
/* Unless we have -device, then USB disks need special
handling */
@@ -3919,26 +4072,6 @@ qemuBuildCommandLine(virConnectPtr conn,
virCommandAddArg(cmd, optstr);
VIR_FREE(optstr);
- if (disk->type == VIR_DOMAIN_DISK_TYPE_NETWORK &&
- disk->protocol == VIR_DOMAIN_DISK_PROTOCOL_RBD) {
- for (j = 0 ; j < disk->nhosts ; j++) {
- if (!has_rbd_hosts) {
- virBufferAddLit(&rbd_hosts, "CEPH_ARGS=-m ");
- has_rbd_hosts = true;
- } else {
- virBufferAddLit(&rbd_hosts, ",");
- }
- virDomainDiskHostDefPtr host = &disk->hosts[j];
- if (host->port) {
- virBufferAsprintf(&rbd_hosts, "%s:%s",
- host->name,
- host->port);
- } else {
- virBufferAdd(&rbd_hosts, host->name, -1);
- }
- }
- }
-
if (!emitBootindex)
bootindex = 0;
else if (disk->bootIndex)
@@ -3976,7 +4109,6 @@ qemuBuildCommandLine(virConnectPtr conn,
char *file;
const char *fmt;
virDomainDiskDefPtr disk = def->disks[i];
- int j;
if (disk->bus == VIR_DOMAIN_DISK_BUS_USB) {
if (disk->device == VIR_DOMAIN_DISK_DEVICE_DISK) {
@@ -4045,24 +4177,15 @@ qemuBuildCommandLine(virConnectPtr conn,
}
break;
case VIR_DOMAIN_DISK_PROTOCOL_RBD:
- if (virAsprintf(&file, "rbd:%s,", disk->src) < 0) {
- goto no_memory;
- }
- for (j = 0 ; j < disk->nhosts ; j++) {
- if (!has_rbd_hosts) {
- virBufferAddLit(&rbd_hosts, "CEPH_ARGS=-m ");
- has_rbd_hosts = true;
- } else {
- virBufferAddLit(&rbd_hosts, ",");
- }
- virDomainDiskHostDefPtr host = &disk->hosts[j];
- if (host->port) {
- virBufferAsprintf(&rbd_hosts, "%s:%s",
- host->name,
- host->port);
- } else {
- virBufferAdd(&rbd_hosts, host->name, -1);
+ {
+ virBuffer opt = VIR_BUFFER_INITIALIZER;
+ if (buildRBDString(conn, disk, &opt) < 0)
+ goto error;
+ if (virBufferError(&opt)) {
+ virReportOOMError();
+ goto error;
}
+ file = virBufferContentAndReset(&opt);
}
break;
case VIR_DOMAIN_DISK_PROTOCOL_SHEEPDOG:
@@ -4091,9 +4214,6 @@ qemuBuildCommandLine(virConnectPtr conn,
}
}
- if (has_rbd_hosts)
- virCommandAddEnvBuffer(cmd, &rbd_hosts);
-
if (qemuCapsGet(qemuCaps, QEMU_CAPS_FSDEV)) {
for (i = 0 ; i < def->nfss ; i++) {
char *optstr;
@@ -5263,7 +5383,6 @@ qemuBuildCommandLine(virConnectPtr conn,
networkReleaseActualDevice(def->nets[i]);
for (i = 0; i <= last_good_net; i++)
virDomainConfNWFilterTeardown(def->nets[i]);
- virBufferFreeAndReset(&rbd_hosts);
virCommandFree(cmd);
return NULL;
}
@@ -5295,10 +5414,6 @@ static int qemuStringToArgvEnv(const char *args,
const char *next;
start = curr;
- /* accept a space in CEPH_ARGS */
- if (STRPREFIX(curr, "CEPH_ARGS=-m ")) {
- start += strlen("CEPH_ARGS=-m ");
- }
if (*start == '\'') {
if (start == curr)
curr++;
@@ -5577,6 +5692,8 @@ qemuParseCommandLineDisk(virCapsPtr caps,
virReportOOMError();
goto cleanup;
}
+ if (parseRBDString(def) < 0)
+ goto cleanup;
VIR_FREE(p);
} else if (STRPREFIX(def->src, "sheepdog:")) {
@@ -6696,7 +6813,8 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps,
disk->src = NULL;
break;
case VIR_DOMAIN_DISK_PROTOCOL_RBD:
- /* handled later since the hosts for all disks are in CEPH_ARGS */
+ if (parseRBDString(disk) < 0)
+ goto error;
break;
case VIR_DOMAIN_DISK_PROTOCOL_SHEEPDOG:
/* disk->src must be [vdiname] or [host]:[port]:[vdiname] */
@@ -7035,68 +7153,6 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps,
}
#undef WANT_VALUE
- if (def->ndisks > 0) {
- const char *ceph_args = qemuFindEnv(progenv, "CEPH_ARGS");
- if (ceph_args) {
- char *hosts, *port, *saveptr = NULL, *token;
- virDomainDiskDefPtr first_rbd_disk = NULL;
- for (i = 0 ; i < def->ndisks ; i++) {
- if (def->disks[i]->type == VIR_DOMAIN_DISK_TYPE_NETWORK &&
- def->disks[i]->protocol == VIR_DOMAIN_DISK_PROTOCOL_RBD) {
- first_rbd_disk = def->disks[i];
- break;
- }
- }
-
- if (!first_rbd_disk) {
- qemuReportError(VIR_ERR_INTERNAL_ERROR,
- _("CEPH_ARGS was set without an rbd disk"));
- goto error;
- }
-
- /* CEPH_ARGS should be: -m host1[:port1][,host2[:port2]]... */
- if (!STRPREFIX(ceph_args, "-m ")) {
- qemuReportError(VIR_ERR_INTERNAL_ERROR,
- _("could not parse CEPH_ARGS '%s'"), ceph_args);
- goto error;
- }
- hosts = strdup(strchr(ceph_args, ' ') + 1);
- if (!hosts)
- goto no_memory;
- first_rbd_disk->nhosts = 0;
- token = strtok_r(hosts, ",", &saveptr);
- while (token != NULL) {
- if (VIR_REALLOC_N(first_rbd_disk->hosts, first_rbd_disk->nhosts + 1) < 0) {
- VIR_FREE(hosts);
- goto no_memory;
- }
- port = strchr(token, ':');
- if (port) {
- *port++ = '\0';
- port = strdup(port);
- if (!port) {
- VIR_FREE(hosts);
- goto no_memory;
- }
- }
- first_rbd_disk->hosts[first_rbd_disk->nhosts].port = port;
- first_rbd_disk->hosts[first_rbd_disk->nhosts].name = strdup(token);
- if (!first_rbd_disk->hosts[first_rbd_disk->nhosts].name) {
- VIR_FREE(hosts);
- goto no_memory;
- }
- first_rbd_disk->nhosts++;
- token = strtok_r(NULL, ",", &saveptr);
- }
- VIR_FREE(hosts);
-
- if (first_rbd_disk->nhosts == 0) {
- qemuReportError(VIR_ERR_INTERNAL_ERROR,
- _("found no rbd hosts in CEPH_ARGS '%s'"), ceph_args);
- goto error;
- }
- }
- }
if (!nographics && def->ngraphics == 0) {
virDomainGraphicsDefPtr sdl;
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-auth.args b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-auth.args
new file mode 100644
index 0000000..f827615
--- /dev/null
+++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-auth.args
@@ -0,0 +1,6 @@
+LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test \
+/usr/bin/qemu -S -M pc -m 214 -smp 1 -nographic -monitor \
+unix:/tmp/test-monitor,server,nowait -no-acpi -boot c -drive \
+file=/dev/HostVG/QEMUGuest1,if=ide,bus=0,unit=0 -drive \
+file=rbd:pool/image:id=myname:key=QVFDVm41aE82SHpGQWhBQXEwTkN2OGp0SmNJY0UrSE9CbE1RMUE=:auth_supported=cephx\;none:mon_host=mon1.example.org\:6321\;mon2.example.org\:6322\;mon3.example.org\:6322,\
+if=virtio,format=raw -net none -serial none -parallel none -usb
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-auth.xml b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-auth.xml
new file mode 100644
index 0000000..88f7f7a
--- /dev/null
+++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-auth.xml
@@ -0,0 +1,37 @@
+<domain type='qemu'>
+ <name>QEMUGuest1</name>
+ <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
+ <memory>219136</memory>
+ <currentMemory>219136</currentMemory>
+ <vcpu>1</vcpu>
+ <os>
+ <type arch='i686' machine='pc'>hvm</type>
+ <boot dev='hd'/>
+ </os>
+ <clock offset='utc'/>
+ <on_poweroff>destroy</on_poweroff>
+ <on_reboot>restart</on_reboot>
+ <on_crash>destroy</on_crash>
+ <devices>
+ <emulator>/usr/bin/qemu</emulator>
+ <disk type='block' device='disk'>
+ <source dev='/dev/HostVG/QEMUGuest1'/>
+ <target dev='hda' bus='ide'/>
+ <address type='drive' controller='0' bus='0' unit='0'/>
+ </disk>
+ <disk type='network' device='disk'>
+ <driver name='qemu' type='raw'/>
+ <auth username='myname'>
+ <secret type='ceph' usage='mycluster_myname'/>
+ </auth>
+ <source protocol='rbd' name='pool/image'>
+ <host name='mon1.example.org' port='6321'/>
+ <host name='mon2.example.org' port='6322'/>
+ <host name='mon3.example.org' port='6322'/>
+ </source>
+ <target dev='vda' bus='virtio'/>
+ </disk>
+ <controller type='ide' index='0'/>
+ <memballoon model='virtio'/>
+ </devices>
+</domain>
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd.args b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd.args
index 3ab1219..50651a5 100644
--- a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd.args
+++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd.args
@@ -1,6 +1,6 @@
-LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test CEPH_ARGS=-m \
-mon1.example.org:6321,mon2.example.org:6322,mon3.example.org:6322 \
+LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test \
/usr/bin/qemu -S -M pc -m 214 -smp 1 -nographic -monitor \
unix:/tmp/test-monitor,server,nowait -no-acpi -boot c -drive \
-file=/dev/HostVG/QEMUGuest1,if=ide,bus=0,unit=0 -drive file=rbd:pool/image,\
+file=/dev/HostVG/QEMUGuest1,if=ide,bus=0,unit=0 -drive \
+file=rbd:pool/image:mon_host=mon1.example.org\:6321\;mon2.example.org\:6322\;mon3.example.org\:6322,\
if=virtio,format=raw -net none -serial none -parallel none -usb
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index a7ae925..31bd928 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -23,6 +23,55 @@
static const char *abs_top_srcdir;
static struct qemud_driver driver;
+static unsigned char *
+fakeSecretGetValue(virSecretPtr obj ATTRIBUTE_UNUSED,
+ size_t *value_size,
+ unsigned int fakeflags ATTRIBUTE_UNUSED,
+ unsigned int internalFlags ATTRIBUTE_UNUSED)
+{
+ char *secret = strdup("AQCVn5hO6HzFAhAAq0NCv8jtJcIcE+HOBlMQ1A");
+ *value_size = strlen(secret);
+ return (unsigned char *) secret;
+}
+
+static virSecretPtr
+fakeSecretLookupByUsage(virConnectPtr conn,
+ int usageType ATTRIBUTE_UNUSED,
+ const char *usageID)
+{
+ virSecretPtr ret = NULL;
+ if (strcmp(usageID, "mycluster_myname"))
+ return ret;
+ ret = malloc(sizeof(virSecret));
+ ret->magic = VIR_SECRET_MAGIC;
+ ret->conn = conn;
+ conn->refs++;
+ ret->refs = 1;
+ ret->usageID = strdup(usageID);
+ return ret;
+}
+
+static int
+fakeSecretClose(virConnectPtr conn ATTRIBUTE_UNUSED)
+{
+ return 0;
+}
+
+static virSecretDriver fakeSecretDriver = {
+ .name = "fake_secret",
+ .open = NULL,
+ .close = fakeSecretClose,
+ .numOfSecrets = NULL,
+ .listSecrets = NULL,
+ .lookupByUUID = NULL,
+ .lookupByUsage = fakeSecretLookupByUsage,
+ .defineXML = NULL,
+ .getXMLDesc = NULL,
+ .setValue = NULL,
+ .getValue = fakeSecretGetValue,
+ .undefine = NULL,
+};
+
static int testCompareXMLToArgvFiles(const char *xml,
const char *cmdline,
virBitmapPtr extraFlags,
@@ -44,6 +93,7 @@ static int testCompareXMLToArgvFiles(const char *xml,
if (!(conn = virGetConnect()))
goto fail;
+ conn->secretDriver = &fakeSecretDriver;
len = virtTestLoadFile(cmdline, &expectargv);
if (len < 0)
@@ -354,6 +404,8 @@ mymain(void)
QEMU_CAPS_DRIVE, QEMU_CAPS_DRIVE_FORMAT);
DO_TEST("disk-drive-network-sheepdog", false,
QEMU_CAPS_DRIVE, QEMU_CAPS_DRIVE_FORMAT);
+ DO_TEST("disk-drive-network-rbd-auth", false,
+ QEMU_CAPS_DRIVE, QEMU_CAPS_DRIVE_FORMAT);
DO_TEST("disk-drive-no-boot", false,
QEMU_CAPS_DRIVE, QEMU_CAPS_DEVICE, QEMU_CAPS_BOOTINDEX);
DO_TEST("disk-usb", false, NONE);
--
1.7.1
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [RFC PATCH v3 4/4] qemu/rbd: improve rbd device specification
2011-10-20 18:01 ` [RFC PATCH v3 4/4] qemu/rbd: improve rbd device specification Josh Durgin
@ 2011-10-27 8:38 ` Daniel P. Berrange
2011-10-28 21:19 ` [PATCH v4 " Josh Durgin
0 siblings, 1 reply; 27+ messages in thread
From: Daniel P. Berrange @ 2011-10-27 8:38 UTC (permalink / raw)
To: Josh Durgin; +Cc: libvir-list, ceph-devel
On Thu, Oct 20, 2011 at 11:01:27AM -0700, Josh Durgin wrote:
> From: Sage Weil <sage@newdream.net>
>
> This improves the support for qemu rbd devices by adding support for a few
> key features (e.g., authentication) and cleaning up the way in which
> rbd configuration options are passed to qemu.
>
> And <auth> member of the disk source xml specifies how librbd should
> authenticate. The username attribute is the Ceph/RBD user to authenticate as.
> The usage or uuid attributes specify which secret to use. Usage is an
> arbitrary identifier local to libvirt.
>
> The old RBD support relied on setting an environment variable to
> communicate information to qemu/librbd. Instead, pass those options
> explicitly to qemu. Update the qemu argument parsing and tests
> accordingly.
>
> Signed-off-by: Sage Weil <sage@newdream.net>
> Signed-off-by: Josh Durgin <josh.durgin@dreamhost.com>
> ---
> src/qemu/qemu_command.c | 284 ++++++++++++--------
> .../qemuxml2argv-disk-drive-network-rbd-auth.args | 6 +
> .../qemuxml2argv-disk-drive-network-rbd-auth.xml | 37 +++
> .../qemuxml2argv-disk-drive-network-rbd.args | 6 +-
> tests/qemuxml2argvtest.c | 52 ++++
> 5 files changed, 268 insertions(+), 117 deletions(-)
> create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-auth.args
> create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-auth.xml
>
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 4dfce88..b2c0eee 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -38,6 +38,7 @@
> #include "domain_audit.h"
> #include "domain_conf.h"
> #include "network/bridge_driver.h"
> +#include "base64.h"
>
> #include <sys/utsname.h>
> #include <sys/stat.h>
> @@ -1489,6 +1490,159 @@ qemuSafeSerialParamValue(const char *value)
> return 0;
> }
>
> +static int buildRBDString(virConnectPtr conn,
> + virDomainDiskDefPtr disk,
> + virBufferPtr opt)
For style reasons, s/buildRBDString/qemuBuildRBDString/
> +{
> + int i;
> + virSecretPtr sec = NULL;
> + char *secret = NULL;
> + size_t secret_size;
> +
> + virBufferAsprintf(opt, "rbd:%s", disk->src);
> + if (disk->auth.username) {
> + virBufferEscape(opt, ":", ":id=%s", disk->auth.username);
> + /* look up secret */
> + switch (disk->auth.secretType) {
> + case VIR_DOMAIN_DISK_SECRET_TYPE_UUID:
> + sec = virSecretLookupByUUID(conn,
> + disk->auth.secret.uuid);
> + break;
> + case VIR_DOMAIN_DISK_SECRET_TYPE_USAGE:
> + sec = virSecretLookupByUsage(conn,
> + VIR_SECRET_USAGE_TYPE_CEPH,
> + disk->auth.secret.usage);
> + break;
> + }
> +
> + if (sec) {
> + char *base64;
> +
> + secret = (char *)conn->secretDriver->getValue(sec, &secret_size, 0,
> + VIR_SECRET_GET_VALUE_INTERNAL_CALL);
> + if (secret == NULL) {
> + qemuReportError(VIR_ERR_INTERNAL_ERROR,
> + _("could not get the value of the secret for username %s"),
> + disk->auth.username);
> + return -1;
> + }
> + /* qemu/librbd wants it base64 encoded */
> + base64_encode_alloc(secret, secret_size, &base64);
> + virBufferEscape(opt, ":", ":key=%s:auth_supported=cephx\\;none",
> + base64);
> + VIR_FREE(base64);
> + VIR_FREE(secret);
> + virUnrefSecret(sec);
> + } else {
> + qemuReportError(VIR_ERR_INTERNAL_ERROR,
> + _("rbd username '%s' specified but secret not found"),
> + disk->auth.username);
> + return -1;
> + }
> + }
> +
> + if (disk->nhosts > 0) {
> + virBufferStrcat(opt, ":mon_host=", NULL);
> + for (i = 0; i < disk->nhosts; ++i) {
> + if (i) {
> + virBufferStrcat(opt, "\\;", NULL);
> + }
> + if (disk->hosts[i].port) {
> + virBufferAsprintf(opt, "%s\\:%s",
> + disk->hosts[i].name,
> + disk->hosts[i].port);
> + } else {
> + virBufferAsprintf(opt, "%s", disk->hosts[i].name);
> + }
> + }
> + }
> +
> + return 0;
> +}
> +
> +static int addRBDHost(virDomainDiskDefPtr disk, char *hostport)
s/add/qemuAdd/
> +{
> + char *port;
> + int ret;
> +
> + disk->nhosts++;
> + ret = VIR_REALLOC_N(disk->hosts, disk->nhosts);
> + if (ret < 0) {
> + virReportOOMError();
> + return -1;
> + }
> +
> + port = strstr(hostport, "\\:");
> + if (port) {
> + *port = '\0';
> + port += 2;
> + disk->hosts[disk->nhosts-1].port = strdup(port);
> + } else {
> + disk->hosts[disk->nhosts-1].port = strdup("6789");
> + }
> + disk->hosts[disk->nhosts-1].name = strdup(hostport);
> + return 0;
> +}
> +
> +/* disk->src initially has everything after the rbd: prefix */
> +static int parseRBDString(virDomainDiskDefPtr disk)
s/parse/qemuParse/
> +{
> + char *options = NULL;
> + char *p, *e, *next;
> +
> + p = strchr(disk->src, ':');
> + if (p) {
> + options = strdup(p + 1);
> + *p = '\0';
> + }
> +
> + /* options */
> + if (!options)
> + return 0; /* all done */
> +
> + p = options;
> + while (*p) {
> + /* find : delimiter or end of string */
> + for (e = p; *e && *e != ':'; ++e) {
> + if (*e == '\\') {
> + e++;
> + if (*e == '\0')
> + break;
> + }
> + }
> + if (*e == '\0') {
> + next = e; /* last kv pair */
> + } else {
> + next = e + 1;
> + *e = '\0';
> + }
> +
> + if (STRPREFIX(p, "id=")) {
> + disk->auth.username = strdup(p + strlen("id="));
> + }
> + if (STRPREFIX(p, "mon_host=")) {
> + char *h, *sep;
> +
> + h = p + strlen("mon_host=");
> + while (h < e) {
> + for (sep = h; sep < e; ++sep) {
> + if (*sep == '\\' && (sep[1] == ',' ||
> + sep[1] == ';' ||
> + sep[1] == ' ')) {
> + *sep = '\0';
> + sep += 2;
> + break;
> + }
> + }
> + addRBDHost(disk, h);
> + h = sep;
> + }
> + }
> +
> + p = next;
> + }
> + return 0;
> +}
>
> char *
> qemuBuildDriveStr(virConnectPtr conn,
> @@ -1608,8 +1762,10 @@ qemuBuildDriveStr(virConnectPtr conn,
> disk->hosts->name, disk->hosts->port);
> break;
> case VIR_DOMAIN_DISK_PROTOCOL_RBD:
> - /* TODO: set monitor hostnames */
> - virBufferAsprintf(&opt, "file=rbd:%s,", disk->src);
> + virBufferStrcat(&opt, "file=", NULL);
> + if (buildRBDString(conn, disk, &opt) < 0)
> + goto error;
> + virBufferStrcat(&opt, ",", NULL);
> break;
> case VIR_DOMAIN_DISK_PROTOCOL_SHEEPDOG:
> if (disk->nhosts == 0)
> @@ -3278,8 +3434,6 @@ qemuBuildCommandLine(virConnectPtr conn,
> int last_good_net = -1;
> bool hasHwVirt = false;
> virCommandPtr cmd;
> - bool has_rbd_hosts = false;
> - virBuffer rbd_hosts = VIR_BUFFER_INITIALIZER;
> bool emitBootindex = false;
> int usbcontroller = 0;
> bool usblegacy = false;
> @@ -3861,7 +4015,6 @@ qemuBuildCommandLine(virConnectPtr conn,
> virDomainDiskDefPtr disk = def->disks[i];
> int withDeviceArg = 0;
> bool deviceFlagMasked = false;
> - int j;
>
> /* Unless we have -device, then USB disks need special
> handling */
> @@ -3919,26 +4072,6 @@ qemuBuildCommandLine(virConnectPtr conn,
> virCommandAddArg(cmd, optstr);
> VIR_FREE(optstr);
>
> - if (disk->type == VIR_DOMAIN_DISK_TYPE_NETWORK &&
> - disk->protocol == VIR_DOMAIN_DISK_PROTOCOL_RBD) {
> - for (j = 0 ; j < disk->nhosts ; j++) {
> - if (!has_rbd_hosts) {
> - virBufferAddLit(&rbd_hosts, "CEPH_ARGS=-m ");
> - has_rbd_hosts = true;
> - } else {
> - virBufferAddLit(&rbd_hosts, ",");
> - }
> - virDomainDiskHostDefPtr host = &disk->hosts[j];
> - if (host->port) {
> - virBufferAsprintf(&rbd_hosts, "%s:%s",
> - host->name,
> - host->port);
> - } else {
> - virBufferAdd(&rbd_hosts, host->name, -1);
> - }
> - }
> - }
> -
> if (!emitBootindex)
> bootindex = 0;
> else if (disk->bootIndex)
> @@ -3976,7 +4109,6 @@ qemuBuildCommandLine(virConnectPtr conn,
> char *file;
> const char *fmt;
> virDomainDiskDefPtr disk = def->disks[i];
> - int j;
>
> if (disk->bus == VIR_DOMAIN_DISK_BUS_USB) {
> if (disk->device == VIR_DOMAIN_DISK_DEVICE_DISK) {
> @@ -4045,24 +4177,15 @@ qemuBuildCommandLine(virConnectPtr conn,
> }
> break;
> case VIR_DOMAIN_DISK_PROTOCOL_RBD:
> - if (virAsprintf(&file, "rbd:%s,", disk->src) < 0) {
> - goto no_memory;
> - }
> - for (j = 0 ; j < disk->nhosts ; j++) {
> - if (!has_rbd_hosts) {
> - virBufferAddLit(&rbd_hosts, "CEPH_ARGS=-m ");
> - has_rbd_hosts = true;
> - } else {
> - virBufferAddLit(&rbd_hosts, ",");
> - }
> - virDomainDiskHostDefPtr host = &disk->hosts[j];
> - if (host->port) {
> - virBufferAsprintf(&rbd_hosts, "%s:%s",
> - host->name,
> - host->port);
> - } else {
> - virBufferAdd(&rbd_hosts, host->name, -1);
> + {
> + virBuffer opt = VIR_BUFFER_INITIALIZER;
> + if (buildRBDString(conn, disk, &opt) < 0)
> + goto error;
> + if (virBufferError(&opt)) {
> + virReportOOMError();
> + goto error;
> }
> + file = virBufferContentAndReset(&opt);
> }
> break;
> case VIR_DOMAIN_DISK_PROTOCOL_SHEEPDOG:
> @@ -4091,9 +4214,6 @@ qemuBuildCommandLine(virConnectPtr conn,
> }
> }
>
> - if (has_rbd_hosts)
> - virCommandAddEnvBuffer(cmd, &rbd_hosts);
> -
> if (qemuCapsGet(qemuCaps, QEMU_CAPS_FSDEV)) {
> for (i = 0 ; i < def->nfss ; i++) {
> char *optstr;
> @@ -5263,7 +5383,6 @@ qemuBuildCommandLine(virConnectPtr conn,
> networkReleaseActualDevice(def->nets[i]);
> for (i = 0; i <= last_good_net; i++)
> virDomainConfNWFilterTeardown(def->nets[i]);
> - virBufferFreeAndReset(&rbd_hosts);
> virCommandFree(cmd);
> return NULL;
> }
> @@ -5295,10 +5414,6 @@ static int qemuStringToArgvEnv(const char *args,
> const char *next;
>
> start = curr;
> - /* accept a space in CEPH_ARGS */
> - if (STRPREFIX(curr, "CEPH_ARGS=-m ")) {
> - start += strlen("CEPH_ARGS=-m ");
> - }
> if (*start == '\'') {
> if (start == curr)
> curr++;
> @@ -5577,6 +5692,8 @@ qemuParseCommandLineDisk(virCapsPtr caps,
> virReportOOMError();
> goto cleanup;
> }
> + if (parseRBDString(def) < 0)
> + goto cleanup;
>
> VIR_FREE(p);
> } else if (STRPREFIX(def->src, "sheepdog:")) {
> @@ -6696,7 +6813,8 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps,
> disk->src = NULL;
> break;
> case VIR_DOMAIN_DISK_PROTOCOL_RBD:
> - /* handled later since the hosts for all disks are in CEPH_ARGS */
> + if (parseRBDString(disk) < 0)
> + goto error;
> break;
> case VIR_DOMAIN_DISK_PROTOCOL_SHEEPDOG:
> /* disk->src must be [vdiname] or [host]:[port]:[vdiname] */
> @@ -7035,68 +7153,6 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps,
> }
>
> #undef WANT_VALUE
> - if (def->ndisks > 0) {
> - const char *ceph_args = qemuFindEnv(progenv, "CEPH_ARGS");
> - if (ceph_args) {
> - char *hosts, *port, *saveptr = NULL, *token;
> - virDomainDiskDefPtr first_rbd_disk = NULL;
> - for (i = 0 ; i < def->ndisks ; i++) {
> - if (def->disks[i]->type == VIR_DOMAIN_DISK_TYPE_NETWORK &&
> - def->disks[i]->protocol == VIR_DOMAIN_DISK_PROTOCOL_RBD) {
> - first_rbd_disk = def->disks[i];
> - break;
> - }
> - }
> -
> - if (!first_rbd_disk) {
> - qemuReportError(VIR_ERR_INTERNAL_ERROR,
> - _("CEPH_ARGS was set without an rbd disk"));
> - goto error;
> - }
> -
> - /* CEPH_ARGS should be: -m host1[:port1][,host2[:port2]]... */
> - if (!STRPREFIX(ceph_args, "-m ")) {
> - qemuReportError(VIR_ERR_INTERNAL_ERROR,
> - _("could not parse CEPH_ARGS '%s'"), ceph_args);
> - goto error;
> - }
> - hosts = strdup(strchr(ceph_args, ' ') + 1);
> - if (!hosts)
> - goto no_memory;
> - first_rbd_disk->nhosts = 0;
> - token = strtok_r(hosts, ",", &saveptr);
> - while (token != NULL) {
> - if (VIR_REALLOC_N(first_rbd_disk->hosts, first_rbd_disk->nhosts + 1) < 0) {
> - VIR_FREE(hosts);
> - goto no_memory;
> - }
> - port = strchr(token, ':');
> - if (port) {
> - *port++ = '\0';
> - port = strdup(port);
> - if (!port) {
> - VIR_FREE(hosts);
> - goto no_memory;
> - }
> - }
> - first_rbd_disk->hosts[first_rbd_disk->nhosts].port = port;
> - first_rbd_disk->hosts[first_rbd_disk->nhosts].name = strdup(token);
> - if (!first_rbd_disk->hosts[first_rbd_disk->nhosts].name) {
> - VIR_FREE(hosts);
> - goto no_memory;
> - }
> - first_rbd_disk->nhosts++;
> - token = strtok_r(NULL, ",", &saveptr);
> - }
> - VIR_FREE(hosts);
> -
> - if (first_rbd_disk->nhosts == 0) {
> - qemuReportError(VIR_ERR_INTERNAL_ERROR,
> - _("found no rbd hosts in CEPH_ARGS '%s'"), ceph_args);
> - goto error;
> - }
> - }
> - }
>
> if (!nographics && def->ngraphics == 0) {
> virDomainGraphicsDefPtr sdl;
> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
> index a7ae925..31bd928 100644
> --- a/tests/qemuxml2argvtest.c
> +++ b/tests/qemuxml2argvtest.c
> @@ -23,6 +23,55 @@
> static const char *abs_top_srcdir;
> static struct qemud_driver driver;
>
> +static unsigned char *
> +fakeSecretGetValue(virSecretPtr obj ATTRIBUTE_UNUSED,
> + size_t *value_size,
> + unsigned int fakeflags ATTRIBUTE_UNUSED,
> + unsigned int internalFlags ATTRIBUTE_UNUSED)
> +{
> + char *secret = strdup("AQCVn5hO6HzFAhAAq0NCv8jtJcIcE+HOBlMQ1A");
> + *value_size = strlen(secret);
> + return (unsigned char *) secret;
> +}
> +
> +static virSecretPtr
> +fakeSecretLookupByUsage(virConnectPtr conn,
> + int usageType ATTRIBUTE_UNUSED,
> + const char *usageID)
> +{
> + virSecretPtr ret = NULL;
> + if (strcmp(usageID, "mycluster_myname"))
s/strcmp/STRNEQ/
> + return ret;
> + ret = malloc(sizeof(virSecret));
Need to use VIR_ALLOC for this.
> + ret->magic = VIR_SECRET_MAGIC;
> + ret->conn = conn;
> + conn->refs++;
> + ret->refs = 1;
> + ret->usageID = strdup(usageID);
> + return ret;
> +}
> +
Regards,
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
^ permalink raw reply [flat|nested] 27+ messages in thread* [PATCH v4 4/4] qemu/rbd: improve rbd device specification
2011-10-27 8:38 ` Daniel P. Berrange
@ 2011-10-28 21:19 ` Josh Durgin
2011-10-31 20:02 ` Eric Blake
0 siblings, 1 reply; 27+ messages in thread
From: Josh Durgin @ 2011-10-28 21:19 UTC (permalink / raw)
To: libvir-list; +Cc: ceph-devel, berrange, eblake
From: Sage Weil <sage@newdream.net>
This improves the support for qemu rbd devices by adding support for a few
key features (e.g., authentication) and cleaning up the way in which
rbd configuration options are passed to qemu.
And <auth> member of the disk source xml specifies how librbd should
authenticate. The username attribute is the Ceph/RBD user to authenticate as.
The usage or uuid attributes specify which secret to use. Usage is an
arbitrary identifier local to libvirt.
The old RBD support relied on setting an environment variable to
communicate information to qemu/librbd. Instead, pass those options
explicitly to qemu. Update the qemu argument parsing and tests
accordingly.
Signed-off-by: Sage Weil <sage@newdream.net>
Signed-off-by: Josh Durgin <josh.durgin@dreamhost.com>
---
This fixes the things Daniel mentioned.
src/qemu/qemu_command.c | 284 ++++++++++++--------
.../qemuxml2argv-disk-drive-network-rbd-auth.args | 6 +
.../qemuxml2argv-disk-drive-network-rbd-auth.xml | 37 +++
.../qemuxml2argv-disk-drive-network-rbd.args | 6 +-
tests/qemuxml2argvtest.c | 56 ++++
5 files changed, 272 insertions(+), 117 deletions(-)
create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-auth.args
create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-auth.xml
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index f5d89b9..48b0762 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -38,6 +38,7 @@
#include "domain_audit.h"
#include "domain_conf.h"
#include "network/bridge_driver.h"
+#include "base64.h"
#include <sys/utsname.h>
#include <sys/stat.h>
@@ -1495,6 +1496,159 @@ qemuSafeSerialParamValue(const char *value)
return 0;
}
+static int qemuBuildRBDString(virConnectPtr conn,
+ virDomainDiskDefPtr disk,
+ virBufferPtr opt)
+{
+ int i;
+ virSecretPtr sec = NULL;
+ char *secret = NULL;
+ size_t secret_size;
+
+ virBufferAsprintf(opt, "rbd:%s", disk->src);
+ if (disk->auth.username) {
+ virBufferEscape(opt, ":", ":id=%s", disk->auth.username);
+ /* look up secret */
+ switch (disk->auth.secretType) {
+ case VIR_DOMAIN_DISK_SECRET_TYPE_UUID:
+ sec = virSecretLookupByUUID(conn,
+ disk->auth.secret.uuid);
+ break;
+ case VIR_DOMAIN_DISK_SECRET_TYPE_USAGE:
+ sec = virSecretLookupByUsage(conn,
+ VIR_SECRET_USAGE_TYPE_CEPH,
+ disk->auth.secret.usage);
+ break;
+ }
+
+ if (sec) {
+ char *base64;
+
+ secret = (char *)conn->secretDriver->getValue(sec, &secret_size, 0,
+ VIR_SECRET_GET_VALUE_INTERNAL_CALL);
+ if (secret == NULL) {
+ qemuReportError(VIR_ERR_INTERNAL_ERROR,
+ _("could not get the value of the secret for username %s"),
+ disk->auth.username);
+ return -1;
+ }
+ /* qemu/librbd wants it base64 encoded */
+ base64_encode_alloc(secret, secret_size, &base64);
+ virBufferEscape(opt, ":", ":key=%s:auth_supported=cephx\\;none",
+ base64);
+ VIR_FREE(base64);
+ VIR_FREE(secret);
+ virUnrefSecret(sec);
+ } else {
+ qemuReportError(VIR_ERR_INTERNAL_ERROR,
+ _("rbd username '%s' specified but secret not found"),
+ disk->auth.username);
+ return -1;
+ }
+ }
+
+ if (disk->nhosts > 0) {
+ virBufferStrcat(opt, ":mon_host=", NULL);
+ for (i = 0; i < disk->nhosts; ++i) {
+ if (i) {
+ virBufferStrcat(opt, "\\;", NULL);
+ }
+ if (disk->hosts[i].port) {
+ virBufferAsprintf(opt, "%s\\:%s",
+ disk->hosts[i].name,
+ disk->hosts[i].port);
+ } else {
+ virBufferAsprintf(opt, "%s", disk->hosts[i].name);
+ }
+ }
+ }
+
+ return 0;
+}
+
+static int qemuAddRBDHost(virDomainDiskDefPtr disk, char *hostport)
+{
+ char *port;
+ int ret;
+
+ disk->nhosts++;
+ ret = VIR_REALLOC_N(disk->hosts, disk->nhosts);
+ if (ret < 0) {
+ virReportOOMError();
+ return -1;
+ }
+
+ port = strstr(hostport, "\\:");
+ if (port) {
+ *port = '\0';
+ port += 2;
+ disk->hosts[disk->nhosts-1].port = strdup(port);
+ } else {
+ disk->hosts[disk->nhosts-1].port = strdup("6789");
+ }
+ disk->hosts[disk->nhosts-1].name = strdup(hostport);
+ return 0;
+}
+
+/* disk->src initially has everything after the rbd: prefix */
+static int qemuParseRBDString(virDomainDiskDefPtr disk)
+{
+ char *options = NULL;
+ char *p, *e, *next;
+
+ p = strchr(disk->src, ':');
+ if (p) {
+ options = strdup(p + 1);
+ *p = '\0';
+ }
+
+ /* options */
+ if (!options)
+ return 0; /* all done */
+
+ p = options;
+ while (*p) {
+ /* find : delimiter or end of string */
+ for (e = p; *e && *e != ':'; ++e) {
+ if (*e == '\\') {
+ e++;
+ if (*e == '\0')
+ break;
+ }
+ }
+ if (*e == '\0') {
+ next = e; /* last kv pair */
+ } else {
+ next = e + 1;
+ *e = '\0';
+ }
+
+ if (STRPREFIX(p, "id=")) {
+ disk->auth.username = strdup(p + strlen("id="));
+ }
+ if (STRPREFIX(p, "mon_host=")) {
+ char *h, *sep;
+
+ h = p + strlen("mon_host=");
+ while (h < e) {
+ for (sep = h; sep < e; ++sep) {
+ if (*sep == '\\' && (sep[1] == ',' ||
+ sep[1] == ';' ||
+ sep[1] == ' ')) {
+ *sep = '\0';
+ sep += 2;
+ break;
+ }
+ }
+ qemuAddRBDHost(disk, h);
+ h = sep;
+ }
+ }
+
+ p = next;
+ }
+ return 0;
+}
char *
qemuBuildDriveStr(virConnectPtr conn,
@@ -1614,8 +1768,10 @@ qemuBuildDriveStr(virConnectPtr conn,
disk->hosts->name, disk->hosts->port);
break;
case VIR_DOMAIN_DISK_PROTOCOL_RBD:
- /* TODO: set monitor hostnames */
- virBufferAsprintf(&opt, "file=rbd:%s,", disk->src);
+ virBufferStrcat(&opt, "file=", NULL);
+ if (qemuBuildRBDString(conn, disk, &opt) < 0)
+ goto error;
+ virBufferStrcat(&opt, ",", NULL);
break;
case VIR_DOMAIN_DISK_PROTOCOL_SHEEPDOG:
if (disk->nhosts == 0)
@@ -3284,8 +3440,6 @@ qemuBuildCommandLine(virConnectPtr conn,
int last_good_net = -1;
bool hasHwVirt = false;
virCommandPtr cmd;
- bool has_rbd_hosts = false;
- virBuffer rbd_hosts = VIR_BUFFER_INITIALIZER;
bool emitBootindex = false;
int usbcontroller = 0;
bool usblegacy = false;
@@ -3865,7 +4019,6 @@ qemuBuildCommandLine(virConnectPtr conn,
virDomainDiskDefPtr disk = def->disks[i];
int withDeviceArg = 0;
bool deviceFlagMasked = false;
- int j;
/* Unless we have -device, then USB disks need special
handling */
@@ -3923,26 +4076,6 @@ qemuBuildCommandLine(virConnectPtr conn,
virCommandAddArg(cmd, optstr);
VIR_FREE(optstr);
- if (disk->type == VIR_DOMAIN_DISK_TYPE_NETWORK &&
- disk->protocol == VIR_DOMAIN_DISK_PROTOCOL_RBD) {
- for (j = 0 ; j < disk->nhosts ; j++) {
- if (!has_rbd_hosts) {
- virBufferAddLit(&rbd_hosts, "CEPH_ARGS=-m ");
- has_rbd_hosts = true;
- } else {
- virBufferAddLit(&rbd_hosts, ",");
- }
- virDomainDiskHostDefPtr host = &disk->hosts[j];
- if (host->port) {
- virBufferAsprintf(&rbd_hosts, "%s:%s",
- host->name,
- host->port);
- } else {
- virBufferAdd(&rbd_hosts, host->name, -1);
- }
- }
- }
-
if (!emitBootindex)
bootindex = 0;
else if (disk->bootIndex)
@@ -3980,7 +4113,6 @@ qemuBuildCommandLine(virConnectPtr conn,
char *file;
const char *fmt;
virDomainDiskDefPtr disk = def->disks[i];
- int j;
if (disk->bus == VIR_DOMAIN_DISK_BUS_USB) {
if (disk->device == VIR_DOMAIN_DISK_DEVICE_DISK) {
@@ -4049,24 +4181,15 @@ qemuBuildCommandLine(virConnectPtr conn,
}
break;
case VIR_DOMAIN_DISK_PROTOCOL_RBD:
- if (virAsprintf(&file, "rbd:%s,", disk->src) < 0) {
- goto no_memory;
- }
- for (j = 0 ; j < disk->nhosts ; j++) {
- if (!has_rbd_hosts) {
- virBufferAddLit(&rbd_hosts, "CEPH_ARGS=-m ");
- has_rbd_hosts = true;
- } else {
- virBufferAddLit(&rbd_hosts, ",");
- }
- virDomainDiskHostDefPtr host = &disk->hosts[j];
- if (host->port) {
- virBufferAsprintf(&rbd_hosts, "%s:%s",
- host->name,
- host->port);
- } else {
- virBufferAdd(&rbd_hosts, host->name, -1);
+ {
+ virBuffer opt = VIR_BUFFER_INITIALIZER;
+ if (qemuBuildRBDString(conn, disk, &opt) < 0)
+ goto error;
+ if (virBufferError(&opt)) {
+ virReportOOMError();
+ goto error;
}
+ file = virBufferContentAndReset(&opt);
}
break;
case VIR_DOMAIN_DISK_PROTOCOL_SHEEPDOG:
@@ -4095,9 +4218,6 @@ qemuBuildCommandLine(virConnectPtr conn,
}
}
- if (has_rbd_hosts)
- virCommandAddEnvBuffer(cmd, &rbd_hosts);
-
if (qemuCapsGet(qemuCaps, QEMU_CAPS_FSDEV)) {
for (i = 0 ; i < def->nfss ; i++) {
char *optstr;
@@ -5267,7 +5387,6 @@ qemuBuildCommandLine(virConnectPtr conn,
networkReleaseActualDevice(def->nets[i]);
for (i = 0; i <= last_good_net; i++)
virDomainConfNWFilterTeardown(def->nets[i]);
- virBufferFreeAndReset(&rbd_hosts);
virCommandFree(cmd);
return NULL;
}
@@ -5299,10 +5418,6 @@ static int qemuStringToArgvEnv(const char *args,
const char *next;
start = curr;
- /* accept a space in CEPH_ARGS */
- if (STRPREFIX(curr, "CEPH_ARGS=-m ")) {
- start += strlen("CEPH_ARGS=-m ");
- }
if (*start == '\'') {
if (start == curr)
curr++;
@@ -5581,6 +5696,8 @@ qemuParseCommandLineDisk(virCapsPtr caps,
virReportOOMError();
goto cleanup;
}
+ if (qemuParseRBDString(def) < 0)
+ goto cleanup;
VIR_FREE(p);
} else if (STRPREFIX(def->src, "sheepdog:")) {
@@ -6700,7 +6817,8 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps,
disk->src = NULL;
break;
case VIR_DOMAIN_DISK_PROTOCOL_RBD:
- /* handled later since the hosts for all disks are in CEPH_ARGS */
+ if (qemuParseRBDString(disk) < 0)
+ goto error;
break;
case VIR_DOMAIN_DISK_PROTOCOL_SHEEPDOG:
/* disk->src must be [vdiname] or [host]:[port]:[vdiname] */
@@ -7039,68 +7157,6 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps,
}
#undef WANT_VALUE
- if (def->ndisks > 0) {
- const char *ceph_args = qemuFindEnv(progenv, "CEPH_ARGS");
- if (ceph_args) {
- char *hosts, *port, *saveptr = NULL, *token;
- virDomainDiskDefPtr first_rbd_disk = NULL;
- for (i = 0 ; i < def->ndisks ; i++) {
- if (def->disks[i]->type == VIR_DOMAIN_DISK_TYPE_NETWORK &&
- def->disks[i]->protocol == VIR_DOMAIN_DISK_PROTOCOL_RBD) {
- first_rbd_disk = def->disks[i];
- break;
- }
- }
-
- if (!first_rbd_disk) {
- qemuReportError(VIR_ERR_INTERNAL_ERROR,
- _("CEPH_ARGS was set without an rbd disk"));
- goto error;
- }
-
- /* CEPH_ARGS should be: -m host1[:port1][,host2[:port2]]... */
- if (!STRPREFIX(ceph_args, "-m ")) {
- qemuReportError(VIR_ERR_INTERNAL_ERROR,
- _("could not parse CEPH_ARGS '%s'"), ceph_args);
- goto error;
- }
- hosts = strdup(strchr(ceph_args, ' ') + 1);
- if (!hosts)
- goto no_memory;
- first_rbd_disk->nhosts = 0;
- token = strtok_r(hosts, ",", &saveptr);
- while (token != NULL) {
- if (VIR_REALLOC_N(first_rbd_disk->hosts, first_rbd_disk->nhosts + 1) < 0) {
- VIR_FREE(hosts);
- goto no_memory;
- }
- port = strchr(token, ':');
- if (port) {
- *port++ = '\0';
- port = strdup(port);
- if (!port) {
- VIR_FREE(hosts);
- goto no_memory;
- }
- }
- first_rbd_disk->hosts[first_rbd_disk->nhosts].port = port;
- first_rbd_disk->hosts[first_rbd_disk->nhosts].name = strdup(token);
- if (!first_rbd_disk->hosts[first_rbd_disk->nhosts].name) {
- VIR_FREE(hosts);
- goto no_memory;
- }
- first_rbd_disk->nhosts++;
- token = strtok_r(NULL, ",", &saveptr);
- }
- VIR_FREE(hosts);
-
- if (first_rbd_disk->nhosts == 0) {
- qemuReportError(VIR_ERR_INTERNAL_ERROR,
- _("found no rbd hosts in CEPH_ARGS '%s'"), ceph_args);
- goto error;
- }
- }
- }
if (!nographics && def->ngraphics == 0) {
virDomainGraphicsDefPtr sdl;
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-auth.args b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-auth.args
new file mode 100644
index 0000000..f827615
--- /dev/null
+++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-auth.args
@@ -0,0 +1,6 @@
+LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test \
+/usr/bin/qemu -S -M pc -m 214 -smp 1 -nographic -monitor \
+unix:/tmp/test-monitor,server,nowait -no-acpi -boot c -drive \
+file=/dev/HostVG/QEMUGuest1,if=ide,bus=0,unit=0 -drive \
+file=rbd:pool/image:id=myname:key=QVFDVm41aE82SHpGQWhBQXEwTkN2OGp0SmNJY0UrSE9CbE1RMUE=:auth_supported=cephx\;none:mon_host=mon1.example.org\:6321\;mon2.example.org\:6322\;mon3.example.org\:6322,\
+if=virtio,format=raw -net none -serial none -parallel none -usb
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-auth.xml b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-auth.xml
new file mode 100644
index 0000000..88f7f7a
--- /dev/null
+++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-auth.xml
@@ -0,0 +1,37 @@
+<domain type='qemu'>
+ <name>QEMUGuest1</name>
+ <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
+ <memory>219136</memory>
+ <currentMemory>219136</currentMemory>
+ <vcpu>1</vcpu>
+ <os>
+ <type arch='i686' machine='pc'>hvm</type>
+ <boot dev='hd'/>
+ </os>
+ <clock offset='utc'/>
+ <on_poweroff>destroy</on_poweroff>
+ <on_reboot>restart</on_reboot>
+ <on_crash>destroy</on_crash>
+ <devices>
+ <emulator>/usr/bin/qemu</emulator>
+ <disk type='block' device='disk'>
+ <source dev='/dev/HostVG/QEMUGuest1'/>
+ <target dev='hda' bus='ide'/>
+ <address type='drive' controller='0' bus='0' unit='0'/>
+ </disk>
+ <disk type='network' device='disk'>
+ <driver name='qemu' type='raw'/>
+ <auth username='myname'>
+ <secret type='ceph' usage='mycluster_myname'/>
+ </auth>
+ <source protocol='rbd' name='pool/image'>
+ <host name='mon1.example.org' port='6321'/>
+ <host name='mon2.example.org' port='6322'/>
+ <host name='mon3.example.org' port='6322'/>
+ </source>
+ <target dev='vda' bus='virtio'/>
+ </disk>
+ <controller type='ide' index='0'/>
+ <memballoon model='virtio'/>
+ </devices>
+</domain>
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd.args b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd.args
index 3ab1219..50651a5 100644
--- a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd.args
+++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd.args
@@ -1,6 +1,6 @@
-LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test CEPH_ARGS=-m \
-mon1.example.org:6321,mon2.example.org:6322,mon3.example.org:6322 \
+LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test \
/usr/bin/qemu -S -M pc -m 214 -smp 1 -nographic -monitor \
unix:/tmp/test-monitor,server,nowait -no-acpi -boot c -drive \
-file=/dev/HostVG/QEMUGuest1,if=ide,bus=0,unit=0 -drive file=rbd:pool/image,\
+file=/dev/HostVG/QEMUGuest1,if=ide,bus=0,unit=0 -drive \
+file=rbd:pool/image:mon_host=mon1.example.org\:6321\;mon2.example.org\:6322\;mon3.example.org\:6322,\
if=virtio,format=raw -net none -serial none -parallel none -usb
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index 4d6db01..34017aa 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -12,6 +12,7 @@
# include "internal.h"
# include "testutils.h"
+# include "util/memory.h"
# include "qemu/qemu_capabilities.h"
# include "qemu/qemu_command.h"
# include "qemu/qemu_domain.h"
@@ -23,6 +24,58 @@
static const char *abs_top_srcdir;
static struct qemud_driver driver;
+static unsigned char *
+fakeSecretGetValue(virSecretPtr obj ATTRIBUTE_UNUSED,
+ size_t *value_size,
+ unsigned int fakeflags ATTRIBUTE_UNUSED,
+ unsigned int internalFlags ATTRIBUTE_UNUSED)
+{
+ char *secret = strdup("AQCVn5hO6HzFAhAAq0NCv8jtJcIcE+HOBlMQ1A");
+ *value_size = strlen(secret);
+ return (unsigned char *) secret;
+}
+
+static virSecretPtr
+fakeSecretLookupByUsage(virConnectPtr conn,
+ int usageType ATTRIBUTE_UNUSED,
+ const char *usageID)
+{
+ virSecretPtr ret = NULL;
+ int err;
+ if (STRNEQ(usageID, "mycluster_myname"))
+ return NULL;
+ err = VIR_ALLOC(ret);
+ if (err < 0)
+ return NULL;
+ ret->magic = VIR_SECRET_MAGIC;
+ ret->conn = conn;
+ conn->refs++;
+ ret->refs = 1;
+ ret->usageID = strdup(usageID);
+ return ret;
+}
+
+static int
+fakeSecretClose(virConnectPtr conn ATTRIBUTE_UNUSED)
+{
+ return 0;
+}
+
+static virSecretDriver fakeSecretDriver = {
+ .name = "fake_secret",
+ .open = NULL,
+ .close = fakeSecretClose,
+ .numOfSecrets = NULL,
+ .listSecrets = NULL,
+ .lookupByUUID = NULL,
+ .lookupByUsage = fakeSecretLookupByUsage,
+ .defineXML = NULL,
+ .getXMLDesc = NULL,
+ .setValue = NULL,
+ .getValue = fakeSecretGetValue,
+ .undefine = NULL,
+};
+
static int testCompareXMLToArgvFiles(const char *xml,
const char *cmdline,
virBitmapPtr extraFlags,
@@ -44,6 +97,7 @@ static int testCompareXMLToArgvFiles(const char *xml,
if (!(conn = virGetConnect()))
goto fail;
+ conn->secretDriver = &fakeSecretDriver;
len = virtTestLoadFile(cmdline, &expectargv);
if (len < 0)
@@ -357,6 +411,8 @@ mymain(void)
QEMU_CAPS_DRIVE, QEMU_CAPS_DRIVE_FORMAT);
DO_TEST("disk-drive-network-sheepdog", false,
QEMU_CAPS_DRIVE, QEMU_CAPS_DRIVE_FORMAT);
+ DO_TEST("disk-drive-network-rbd-auth", false,
+ QEMU_CAPS_DRIVE, QEMU_CAPS_DRIVE_FORMAT);
DO_TEST("disk-drive-no-boot", false,
QEMU_CAPS_DRIVE, QEMU_CAPS_DEVICE, QEMU_CAPS_BOOTINDEX);
DO_TEST("disk-usb", false, NONE);
--
1.7.1
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH v4 4/4] qemu/rbd: improve rbd device specification
2011-10-28 21:19 ` [PATCH v4 " Josh Durgin
@ 2011-10-31 20:02 ` Eric Blake
2011-11-01 1:29 ` [PATCH v5 " Josh Durgin
0 siblings, 1 reply; 27+ messages in thread
From: Eric Blake @ 2011-10-31 20:02 UTC (permalink / raw)
To: Josh Durgin; +Cc: libvir-list, ceph-devel, berrange
On 10/28/2011 03:19 PM, Josh Durgin wrote:
> From: Sage Weil<sage@newdream.net>
>
> This improves the support for qemu rbd devices by adding support for a few
> key features (e.g., authentication) and cleaning up the way in which
> rbd configuration options are passed to qemu.
>
> And<auth> member of the disk source xml specifies how librbd should
s/And/An/
> authenticate. The username attribute is the Ceph/RBD user to authenticate as.
> The usage or uuid attributes specify which secret to use. Usage is an
> arbitrary identifier local to libvirt.
>
> The old RBD support relied on setting an environment variable to
> communicate information to qemu/librbd. Instead, pass those options
> explicitly to qemu. Update the qemu argument parsing and tests
> accordingly.
>
> Signed-off-by: Sage Weil<sage@newdream.net>
> Signed-off-by: Josh Durgin<josh.durgin@dreamhost.com>
> ---
>
> This fixes the things Daniel mentioned.
Needs a v5, to fix memory management issues.
> @@ -1495,6 +1496,159 @@ qemuSafeSerialParamValue(const char *value)
> return 0;
> }
>
> +static int qemuBuildRBDString(virConnectPtr conn,
> + virDomainDiskDefPtr disk,
> + virBufferPtr opt)
Nit: We've been using this style:
static int
qemuBuildRBDString(virConnectPtr conn,
virDomainDiskDefPtr disk,
virBufferPtr opt)
> + if (sec) {
> + char *base64;
> +
> + secret = (char *)conn->secretDriver->getValue(sec,&secret_size, 0,
> + VIR_SECRET_GET_VALUE_INTERNAL_CALL);
> + if (secret == NULL) {
> + qemuReportError(VIR_ERR_INTERNAL_ERROR,
> + _("could not get the value of the secret for username %s"),
> + disk->auth.username);
> + return -1;
Resource leak. You need to ensure virUnrefSecret(sec) gets called on
this path. I'd move that particular cleanup down into the cleanup:
label, and make this part goto cleanup instead of return -1.
> + }
> + /* qemu/librbd wants it base64 encoded */
> + base64_encode_alloc(secret, secret_size,&base64);
> + virBufferEscape(opt, ":", ":key=%s:auth_supported=cephx\\;none",
> + base64);
Need to check for allocation failure of base64_encode_alloc; otherwise,
you blindly passed NULL base64 to virBufferEscape, which is not portable.
> + VIR_FREE(base64);
> + VIR_FREE(secret);
> + virUnrefSecret(sec);
> + } else {
> + qemuReportError(VIR_ERR_INTERNAL_ERROR,
> + _("rbd username '%s' specified but secret not found"),
> + disk->auth.username);
> + return -1;
> + }
> + }
> +
> + if (disk->nhosts> 0) {
> + virBufferStrcat(opt, ":mon_host=", NULL);
> + for (i = 0; i< disk->nhosts; ++i) {
> + if (i) {
> + virBufferStrcat(opt, "\\;", NULL);
virBufferAddLit(opt, "\\;")
is more efficient than virBufferStrcat().
> + }
> + if (disk->hosts[i].port) {
> + virBufferAsprintf(opt, "%s\\:%s",
> + disk->hosts[i].name,
> + disk->hosts[i].port);
> + } else {
> + virBufferAsprintf(opt, "%s", disk->hosts[i].name);
> + }
> + }
> + }
> +
> + return 0;
> +}
> +
> +static int qemuAddRBDHost(virDomainDiskDefPtr disk, char *hostport)
> +{
> + char *port;
> + int ret;
> +
> + disk->nhosts++;
> + ret = VIR_REALLOC_N(disk->hosts, disk->nhosts);
> + if (ret< 0) {
> + virReportOOMError();
> + return -1;
> + }
> +
> + port = strstr(hostport, "\\:");
> + if (port) {
> + *port = '\0';
> + port += 2;
> + disk->hosts[disk->nhosts-1].port = strdup(port);
> + } else {
> + disk->hosts[disk->nhosts-1].port = strdup("6789");
> + }
> + disk->hosts[disk->nhosts-1].name = strdup(hostport);
These three strdup() need to check for allocation failure.
> + return 0;
> +}
> +
> +/* disk->src initially has everything after the rbd: prefix */
> +static int qemuParseRBDString(virDomainDiskDefPtr disk)
> +{
> + char *options = NULL;
> + char *p, *e, *next;
> +
> + p = strchr(disk->src, ':');
> + if (p) {
> + options = strdup(p + 1);
> + *p = '\0';
Need to check for strdup() failure.
> + }
> +
> + /* options */
> + if (!options)
> + return 0; /* all done */
> +
> + p = options;
> + while (*p) {
> + /* find : delimiter or end of string */
> + for (e = p; *e&& *e != ':'; ++e) {
> + if (*e == '\\') {
> + e++;
> + if (*e == '\0')
> + break;
> + }
> + }
> + if (*e == '\0') {
> + next = e; /* last kv pair */
> + } else {
> + next = e + 1;
> + *e = '\0';
> + }
> +
> + if (STRPREFIX(p, "id=")) {
> + disk->auth.username = strdup(p + strlen("id="));
Check for allocation failure. Also, you might want to see if using
strndup() on the original string is easier than copying the string, just
so you can do in-place modifications of injecting NUL bytes for strdup()
to work.
> + }
> + if (STRPREFIX(p, "mon_host=")) {
> + char *h, *sep;
> +
> + h = p + strlen("mon_host=");
> + while (h< e) {
> + for (sep = h; sep< e; ++sep) {
> + if (*sep == '\\'&& (sep[1] == ',' ||
> + sep[1] == ';' ||
> + sep[1] == ' ')) {
> + *sep = '\0';
> + sep += 2;
> + break;
> + }
> + }
> + qemuAddRBDHost(disk, h);
Don't ignore the return value here.
> + h = sep;
> + }
> + }
> +
> + p = next;
> + }
> + return 0;
> +}
>
> char *
> qemuBuildDriveStr(virConnectPtr conn,
> @@ -1614,8 +1768,10 @@ qemuBuildDriveStr(virConnectPtr conn,
> disk->hosts->name, disk->hosts->port);
> break;
> case VIR_DOMAIN_DISK_PROTOCOL_RBD:
> - /* TODO: set monitor hostnames */
> - virBufferAsprintf(&opt, "file=rbd:%s,", disk->src);
> + virBufferStrcat(&opt, "file=", NULL);
virBufferAddLit(&opt, "file=") is more efficient.
> + if (qemuBuildRBDString(conn, disk,&opt)< 0)
> + goto error;
> + virBufferStrcat(&opt, ",", NULL);
virBufferAddChar(&opt, ',') is more efficient.
> @@ -5299,10 +5418,6 @@ static int qemuStringToArgvEnv(const char *args,
> const char *next;
>
> start = curr;
> - /* accept a space in CEPH_ARGS */
> - if (STRPREFIX(curr, "CEPH_ARGS=-m ")) {
> - start += strlen("CEPH_ARGS=-m ");
> - }
While we aren't generating CEPH_ARGS in the environment any more, I
don't think we should rip out the parsing code. That is, parsing should
be able to handle our older output, in addition to our new output.
> +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-auth.args
> @@ -0,0 +1,6 @@
> +LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test \
> +/usr/bin/qemu -S -M pc -m 214 -smp 1 -nographic -monitor \
> +unix:/tmp/test-monitor,server,nowait -no-acpi -boot c -drive \
> +file=/dev/HostVG/QEMUGuest1,if=ide,bus=0,unit=0 -drive \
> +file=rbd:pool/image:id=myname:key=QVFDVm41aE82SHpGQWhBQXEwTkN2OGp0SmNJY0UrSE9CbE1RMUE=:auth_supported=cephx\;none:mon_host=mon1.example.org\:6321\;mon2.example.org\:6322\;mon3.example.org\:6322,\
That's a rather long line - I'd suggest using backslash-newline to break
it up, probably at each ':' (it's not the end of the world if you go
longer than 80 columns, if absolutely necessary, but life is easier when
everything just fits).
> +static virSecretPtr
> +fakeSecretLookupByUsage(virConnectPtr conn,
> + int usageType ATTRIBUTE_UNUSED,
> + const char *usageID)
> +{
> + virSecretPtr ret = NULL;
> + int err;
> + if (STRNEQ(usageID, "mycluster_myname"))
> + return NULL;
> + err = VIR_ALLOC(ret);
> + if (err< 0)
> + return NULL;
> + ret->magic = VIR_SECRET_MAGIC;
> + ret->conn = conn;
> + conn->refs++;
> + ret->refs = 1;
> + ret->usageID = strdup(usageID);
Check for strdup() failure.
--
Eric Blake eblake@redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
^ permalink raw reply [flat|nested] 27+ messages in thread* [PATCH v5 4/4] qemu/rbd: improve rbd device specification
2011-10-31 20:02 ` Eric Blake
@ 2011-11-01 1:29 ` Josh Durgin
2011-11-16 0:05 ` Eric Blake
0 siblings, 1 reply; 27+ messages in thread
From: Josh Durgin @ 2011-11-01 1:29 UTC (permalink / raw)
To: libvir-list; +Cc: ceph-devel, eblake
From: Sage Weil <sage@newdream.net>
This improves the support for qemu rbd devices by adding support for a few
key features (e.g., authentication) and cleaning up the way in which
rbd configuration options are passed to qemu.
An <auth> member of the disk source xml specifies how librbd should
authenticate. The username attribute is the Ceph/RBD user to authenticate as.
The usage or uuid attributes specify which secret to use. Usage is an
arbitrary identifier local to libvirt.
The old RBD support relied on setting an environment variable to
communicate information to qemu/librbd. Instead, pass those options
explicitly to qemu. Update the qemu argument parsing and tests
accordingly.
Signed-off-by: Sage Weil <sage@newdream.net>
Signed-off-by: Josh Durgin <josh.durgin@dreamhost.com>
---
Changes since v4:
* fixes memory management issues
* keep older rbd command line parsing and test case
* check qemuAddRBDHost return values
* use more efficient virBuffer functions
src/qemu/qemu_command.c | 356 ++++++++++++++------
tests/qemuargv2xmltest.c | 2 +
.../qemuxml2argv-disk-drive-network-rbd-auth.args | 10 +
.../qemuxml2argv-disk-drive-network-rbd-auth.xml | 37 ++
...muxml2argv-disk-drive-network-rbd-ceph-env.args | 6 +
...emuxml2argv-disk-drive-network-rbd-ceph-env.xml | 34 ++
.../qemuxml2argv-disk-drive-network-rbd.args | 7 +-
tests/qemuxml2argvtest.c | 58 ++++
8 files changed, 406 insertions(+), 104 deletions(-)
create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-auth.args
create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-auth.xml
create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-ceph-env.args
create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-ceph-env.xml
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index dc92fa3..55859e2 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -38,6 +38,7 @@
#include "domain_audit.h"
#include "domain_conf.h"
#include "network/bridge_driver.h"
+#include "base64.h"
#include <sys/utsname.h>
#include <sys/stat.h>
@@ -1495,6 +1496,189 @@ qemuSafeSerialParamValue(const char *value)
return 0;
}
+static int
+qemuBuildRBDString(virConnectPtr conn,
+ virDomainDiskDefPtr disk,
+ virBufferPtr opt)
+{
+ int i, ret = 0;
+ virSecretPtr sec = NULL;
+ char *secret = NULL;
+ size_t secret_size;
+
+ virBufferAsprintf(opt, "rbd:%s", disk->src);
+ if (disk->auth.username) {
+ virBufferEscape(opt, ":", ":id=%s", disk->auth.username);
+ /* look up secret */
+ switch (disk->auth.secretType) {
+ case VIR_DOMAIN_DISK_SECRET_TYPE_UUID:
+ sec = virSecretLookupByUUID(conn,
+ disk->auth.secret.uuid);
+ break;
+ case VIR_DOMAIN_DISK_SECRET_TYPE_USAGE:
+ sec = virSecretLookupByUsage(conn,
+ VIR_SECRET_USAGE_TYPE_CEPH,
+ disk->auth.secret.usage);
+ break;
+ }
+
+ if (sec) {
+ char *base64 = NULL;
+
+ secret = (char *)conn->secretDriver->getValue(sec, &secret_size, 0,
+ VIR_SECRET_GET_VALUE_INTERNAL_CALL);
+ if (secret == NULL) {
+ qemuReportError(VIR_ERR_INTERNAL_ERROR,
+ _("could not get the value of the secret for username %s"),
+ disk->auth.username);
+ goto error;
+ }
+ /* qemu/librbd wants it base64 encoded */
+ base64_encode_alloc(secret, secret_size, &base64);
+ if (!base64) {
+ virReportOOMError();
+ goto error;
+ }
+ virBufferEscape(opt, ":", ":key=%s:auth_supported=cephx none",
+ base64);
+ VIR_FREE(base64);
+ } else {
+ qemuReportError(VIR_ERR_INTERNAL_ERROR,
+ _("rbd username '%s' specified but secret not found"),
+ disk->auth.username);
+ goto error;
+ }
+ }
+
+ if (disk->nhosts > 0) {
+ virBufferAddLit(opt, ":mon_host=");
+ for (i = 0; i < disk->nhosts; ++i) {
+ if (i) {
+ virBufferAddLit(opt, "\\;");
+ }
+ if (disk->hosts[i].port) {
+ virBufferAsprintf(opt, "%s\\:%s",
+ disk->hosts[i].name,
+ disk->hosts[i].port);
+ } else {
+ virBufferAsprintf(opt, "%s", disk->hosts[i].name);
+ }
+ }
+ }
+
+cleanup:
+ VIR_FREE(secret);
+ if (sec)
+ virUnrefSecret(sec);
+
+ return ret;
+
+error:
+ ret = -1;
+ goto cleanup;
+}
+
+static int qemuAddRBDHost(virDomainDiskDefPtr disk, char *hostport)
+{
+ char *port;
+
+ disk->nhosts++;
+ if (VIR_REALLOC_N(disk->hosts, disk->nhosts) < 0)
+ goto no_memory;
+
+ port = strstr(hostport, "\\:");
+ if (port) {
+ *port = '\0';
+ port += 2;
+ disk->hosts[disk->nhosts-1].port = strdup(port);
+ if (!disk->hosts[disk->nhosts-1].port)
+ goto no_memory;
+ } else {
+ disk->hosts[disk->nhosts-1].port = strdup("6789");
+ if (!disk->hosts[disk->nhosts-1].port)
+ goto no_memory;
+ }
+ disk->hosts[disk->nhosts-1].name = strdup(hostport);
+ if (!disk->hosts[disk->nhosts-1].name)
+ goto no_memory;
+ return 0;
+
+no_memory:
+ virReportOOMError();
+ VIR_FREE(disk->hosts[disk->nhosts-1].port);
+ VIR_FREE(disk->hosts[disk->nhosts-1].name);
+ return -1;
+}
+
+/* disk->src initially has everything after the rbd: prefix */
+static int qemuParseRBDString(virDomainDiskDefPtr disk)
+{
+ char *options = NULL;
+ char *p, *e, *next;
+
+ p = strchr(disk->src, ':');
+ if (p) {
+ options = strdup(p + 1);
+ if (!options)
+ goto no_memory;
+ *p = '\0';
+ }
+
+ /* options */
+ if (!options)
+ return 0; /* all done */
+
+ p = options;
+ while (*p) {
+ /* find : delimiter or end of string */
+ for (e = p; *e && *e != ':'; ++e) {
+ if (*e == '\\') {
+ e++;
+ if (*e == '\0')
+ break;
+ }
+ }
+ if (*e == '\0') {
+ next = e; /* last kv pair */
+ } else {
+ next = e + 1;
+ *e = '\0';
+ }
+
+ if (STRPREFIX(p, "id=")) {
+ disk->auth.username = strdup(p + strlen("id="));
+ if (!disk->auth.username)
+ goto no_memory;
+ }
+ if (STRPREFIX(p, "mon_host=")) {
+ char *h, *sep;
+
+ h = p + strlen("mon_host=");
+ while (h < e) {
+ for (sep = h; sep < e; ++sep) {
+ if (*sep == '\\' && (sep[1] == ',' ||
+ sep[1] == ';' ||
+ sep[1] == ' ')) {
+ *sep = '\0';
+ sep += 2;
+ break;
+ }
+ }
+ if (qemuAddRBDHost(disk, h) < 0) {
+ return -1;
+ }
+ h = sep;
+ }
+ }
+
+ p = next;
+ }
+ return 0;
+
+no_memory:
+ virReportOOMError();
+ return -1;
+}
char *
qemuBuildDriveStr(virConnectPtr conn ATTRIBUTE_UNUSED,
@@ -1614,8 +1798,10 @@ qemuBuildDriveStr(virConnectPtr conn ATTRIBUTE_UNUSED,
disk->hosts->name, disk->hosts->port);
break;
case VIR_DOMAIN_DISK_PROTOCOL_RBD:
- /* TODO: set monitor hostnames */
- virBufferAsprintf(&opt, "file=rbd:%s,", disk->src);
+ virBufferAddLit(&opt, "file=");
+ if (qemuBuildRBDString(conn, disk, &opt) < 0)
+ goto error;
+ virBufferAddChar(&opt, ',');
break;
case VIR_DOMAIN_DISK_PROTOCOL_SHEEPDOG:
if (disk->nhosts == 0)
@@ -3284,8 +3470,6 @@ qemuBuildCommandLine(virConnectPtr conn,
int last_good_net = -1;
bool hasHwVirt = false;
virCommandPtr cmd;
- bool has_rbd_hosts = false;
- virBuffer rbd_hosts = VIR_BUFFER_INITIALIZER;
bool emitBootindex = false;
int usbcontroller = 0;
bool usblegacy = false;
@@ -3865,7 +4049,6 @@ qemuBuildCommandLine(virConnectPtr conn,
virDomainDiskDefPtr disk = def->disks[i];
int withDeviceArg = 0;
bool deviceFlagMasked = false;
- int j;
/* Unless we have -device, then USB disks need special
handling */
@@ -3923,26 +4106,6 @@ qemuBuildCommandLine(virConnectPtr conn,
virCommandAddArg(cmd, optstr);
VIR_FREE(optstr);
- if (disk->type == VIR_DOMAIN_DISK_TYPE_NETWORK &&
- disk->protocol == VIR_DOMAIN_DISK_PROTOCOL_RBD) {
- for (j = 0 ; j < disk->nhosts ; j++) {
- if (!has_rbd_hosts) {
- virBufferAddLit(&rbd_hosts, "CEPH_ARGS=-m ");
- has_rbd_hosts = true;
- } else {
- virBufferAddLit(&rbd_hosts, ",");
- }
- virDomainDiskHostDefPtr host = &disk->hosts[j];
- if (host->port) {
- virBufferAsprintf(&rbd_hosts, "%s:%s",
- host->name,
- host->port);
- } else {
- virBufferAdd(&rbd_hosts, host->name, -1);
- }
- }
- }
-
if (!emitBootindex)
bootindex = 0;
else if (disk->bootIndex)
@@ -3980,7 +4143,6 @@ qemuBuildCommandLine(virConnectPtr conn,
char *file;
const char *fmt;
virDomainDiskDefPtr disk = def->disks[i];
- int j;
if (disk->bus == VIR_DOMAIN_DISK_BUS_USB) {
if (disk->device == VIR_DOMAIN_DISK_DEVICE_DISK) {
@@ -4049,24 +4211,15 @@ qemuBuildCommandLine(virConnectPtr conn,
}
break;
case VIR_DOMAIN_DISK_PROTOCOL_RBD:
- if (virAsprintf(&file, "rbd:%s,", disk->src) < 0) {
- goto no_memory;
- }
- for (j = 0 ; j < disk->nhosts ; j++) {
- if (!has_rbd_hosts) {
- virBufferAddLit(&rbd_hosts, "CEPH_ARGS=-m ");
- has_rbd_hosts = true;
- } else {
- virBufferAddLit(&rbd_hosts, ",");
- }
- virDomainDiskHostDefPtr host = &disk->hosts[j];
- if (host->port) {
- virBufferAsprintf(&rbd_hosts, "%s:%s",
- host->name,
- host->port);
- } else {
- virBufferAdd(&rbd_hosts, host->name, -1);
+ {
+ virBuffer opt = VIR_BUFFER_INITIALIZER;
+ if (qemuBuildRBDString(conn, disk, &opt) < 0)
+ goto error;
+ if (virBufferError(&opt)) {
+ virReportOOMError();
+ goto error;
}
+ file = virBufferContentAndReset(&opt);
}
break;
case VIR_DOMAIN_DISK_PROTOCOL_SHEEPDOG:
@@ -4095,9 +4248,6 @@ qemuBuildCommandLine(virConnectPtr conn,
}
}
- if (has_rbd_hosts)
- virCommandAddEnvBuffer(cmd, &rbd_hosts);
-
if (qemuCapsGet(qemuCaps, QEMU_CAPS_FSDEV)) {
for (i = 0 ; i < def->nfss ; i++) {
char *optstr;
@@ -5268,7 +5418,6 @@ qemuBuildCommandLine(virConnectPtr conn,
networkReleaseActualDevice(def->nets[i]);
for (i = 0; i <= last_good_net; i++)
virDomainConfNWFilterTeardown(def->nets[i]);
- virBufferFreeAndReset(&rbd_hosts);
virCommandFree(cmd);
return NULL;
}
@@ -5507,7 +5656,8 @@ error:
static virDomainDiskDefPtr
qemuParseCommandLineDisk(virCapsPtr caps,
const char *val,
- int nvirtiodisk)
+ int nvirtiodisk,
+ bool old_style_ceph_args)
{
virDomainDiskDefPtr def = NULL;
char **keywords;
@@ -5582,6 +5732,9 @@ qemuParseCommandLineDisk(virCapsPtr caps,
virReportOOMError();
goto cleanup;
}
+ /* old-style CEPH_ARGS env variable is parsed later */
+ if (!old_style_ceph_args && qemuParseRBDString(def) < 0)
+ goto cleanup;
VIR_FREE(p);
} else if (STRPREFIX(def->src, "sheepdog:")) {
@@ -6447,6 +6600,7 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps,
int nvirtiodisk = 0;
qemuDomainCmdlineDefPtr cmd = NULL;
virDomainDiskDefPtr disk = NULL;
+ const char *ceph_args = qemuFindEnv(progenv, "CEPH_ARGS");
if (pidfile)
*pidfile = NULL;
@@ -6701,7 +6855,9 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps,
disk->src = NULL;
break;
case VIR_DOMAIN_DISK_PROTOCOL_RBD:
- /* handled later since the hosts for all disks are in CEPH_ARGS */
+ /* old-style CEPH_ARGS env variable is parsed later */
+ if (!ceph_args && qemuParseRBDString(disk) < 0)
+ goto error;
break;
case VIR_DOMAIN_DISK_PROTOCOL_SHEEPDOG:
/* disk->src must be [vdiname] or [host]:[port]:[vdiname] */
@@ -6901,7 +7057,8 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps,
}
} else if (STREQ(arg, "-drive")) {
WANT_VALUE();
- if (!(disk = qemuParseCommandLineDisk(caps, val, nvirtiodisk)))
+ if (!(disk = qemuParseCommandLineDisk(caps, val, nvirtiodisk,
+ ceph_args != NULL)))
goto error;
if (VIR_REALLOC_N(def->disks, def->ndisks+1) < 0)
goto no_memory;
@@ -7040,66 +7197,63 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps,
}
#undef WANT_VALUE
- if (def->ndisks > 0) {
- const char *ceph_args = qemuFindEnv(progenv, "CEPH_ARGS");
- if (ceph_args) {
- char *hosts, *port, *saveptr = NULL, *token;
- virDomainDiskDefPtr first_rbd_disk = NULL;
- for (i = 0 ; i < def->ndisks ; i++) {
- if (def->disks[i]->type == VIR_DOMAIN_DISK_TYPE_NETWORK &&
- def->disks[i]->protocol == VIR_DOMAIN_DISK_PROTOCOL_RBD) {
- first_rbd_disk = def->disks[i];
- break;
- }
+ if (def->ndisks > 0 && ceph_args) {
+ char *hosts, *port, *saveptr = NULL, *token;
+ virDomainDiskDefPtr first_rbd_disk = NULL;
+ for (i = 0 ; i < def->ndisks ; i++) {
+ if (def->disks[i]->type == VIR_DOMAIN_DISK_TYPE_NETWORK &&
+ def->disks[i]->protocol == VIR_DOMAIN_DISK_PROTOCOL_RBD) {
+ first_rbd_disk = def->disks[i];
+ break;
}
+ }
- if (!first_rbd_disk) {
- qemuReportError(VIR_ERR_INTERNAL_ERROR,
- _("CEPH_ARGS was set without an rbd disk"));
- goto error;
- }
+ if (!first_rbd_disk) {
+ qemuReportError(VIR_ERR_INTERNAL_ERROR,
+ _("CEPH_ARGS was set without an rbd disk"));
+ goto error;
+ }
- /* CEPH_ARGS should be: -m host1[:port1][,host2[:port2]]... */
- if (!STRPREFIX(ceph_args, "-m ")) {
- qemuReportError(VIR_ERR_INTERNAL_ERROR,
- _("could not parse CEPH_ARGS '%s'"), ceph_args);
- goto error;
- }
- hosts = strdup(strchr(ceph_args, ' ') + 1);
- if (!hosts)
+ /* CEPH_ARGS should be: -m host1[:port1][,host2[:port2]]... */
+ if (!STRPREFIX(ceph_args, "-m ")) {
+ qemuReportError(VIR_ERR_INTERNAL_ERROR,
+ _("could not parse CEPH_ARGS '%s'"), ceph_args);
+ goto error;
+ }
+ hosts = strdup(strchr(ceph_args, ' ') + 1);
+ if (!hosts)
+ goto no_memory;
+ first_rbd_disk->nhosts = 0;
+ token = strtok_r(hosts, ",", &saveptr);
+ while (token != NULL) {
+ if (VIR_REALLOC_N(first_rbd_disk->hosts, first_rbd_disk->nhosts + 1) < 0) {
+ VIR_FREE(hosts);
goto no_memory;
- first_rbd_disk->nhosts = 0;
- token = strtok_r(hosts, ",", &saveptr);
- while (token != NULL) {
- if (VIR_REALLOC_N(first_rbd_disk->hosts, first_rbd_disk->nhosts + 1) < 0) {
- VIR_FREE(hosts);
- goto no_memory;
- }
- port = strchr(token, ':');
- if (port) {
- *port++ = '\0';
- port = strdup(port);
- if (!port) {
- VIR_FREE(hosts);
- goto no_memory;
- }
- }
- first_rbd_disk->hosts[first_rbd_disk->nhosts].port = port;
- first_rbd_disk->hosts[first_rbd_disk->nhosts].name = strdup(token);
- if (!first_rbd_disk->hosts[first_rbd_disk->nhosts].name) {
+ }
+ port = strchr(token, ':');
+ if (port) {
+ *port++ = '\0';
+ port = strdup(port);
+ if (!port) {
VIR_FREE(hosts);
goto no_memory;
}
- first_rbd_disk->nhosts++;
- token = strtok_r(NULL, ",", &saveptr);
}
- VIR_FREE(hosts);
-
- if (first_rbd_disk->nhosts == 0) {
- qemuReportError(VIR_ERR_INTERNAL_ERROR,
- _("found no rbd hosts in CEPH_ARGS '%s'"), ceph_args);
- goto error;
+ first_rbd_disk->hosts[first_rbd_disk->nhosts].port = port;
+ first_rbd_disk->hosts[first_rbd_disk->nhosts].name = strdup(token);
+ if (!first_rbd_disk->hosts[first_rbd_disk->nhosts].name) {
+ VIR_FREE(hosts);
+ goto no_memory;
}
+ first_rbd_disk->nhosts++;
+ token = strtok_r(NULL, ",", &saveptr);
+ }
+ VIR_FREE(hosts);
+
+ if (first_rbd_disk->nhosts == 0) {
+ qemuReportError(VIR_ERR_INTERNAL_ERROR,
+ _("found no rbd hosts in CEPH_ARGS '%s'"), ceph_args);
+ goto error;
}
}
diff --git a/tests/qemuargv2xmltest.c b/tests/qemuargv2xmltest.c
index 807a771..a20df06 100644
--- a/tests/qemuargv2xmltest.c
+++ b/tests/qemuargv2xmltest.c
@@ -173,6 +173,8 @@ mymain(void)
DO_TEST("disk-drive-cache-unsafe");
DO_TEST("disk-drive-network-nbd");
DO_TEST("disk-drive-network-rbd");
+ /* older format using CEPH_ARGS env var */
+ DO_TEST("disk-drive-network-rbd-ceph-env");
DO_TEST("disk-drive-network-sheepdog");
DO_TEST("disk-usb");
DO_TEST("graphics-vnc");
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-auth.args b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-auth.args
new file mode 100644
index 0000000..1500672
--- /dev/null
+++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-auth.args
@@ -0,0 +1,10 @@
+LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test \
+/usr/bin/qemu -S -M pc -m 214 -smp 1 -nographic -monitor \
+unix:/tmp/test-monitor,server,nowait -no-acpi -boot c -drive \
+file=/dev/HostVG/QEMUGuest1,if=ide,bus=0,unit=0 -drive \
+file=rbd:pool/image:\
+id=myname:\
+key=QVFDVm41aE82SHpGQWhBQXEwTkN2OGp0SmNJY0UrSE9CbE1RMUE=:\
+auth_supported=cephx none:\
+mon_host=mon1.example.org\:6321\;mon2.example.org\:6322\;mon3.example.org\:6322,\
+if=virtio,format=raw -net none -serial none -parallel none -usb
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-auth.xml b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-auth.xml
new file mode 100644
index 0000000..88f7f7a
--- /dev/null
+++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-auth.xml
@@ -0,0 +1,37 @@
+<domain type='qemu'>
+ <name>QEMUGuest1</name>
+ <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
+ <memory>219136</memory>
+ <currentMemory>219136</currentMemory>
+ <vcpu>1</vcpu>
+ <os>
+ <type arch='i686' machine='pc'>hvm</type>
+ <boot dev='hd'/>
+ </os>
+ <clock offset='utc'/>
+ <on_poweroff>destroy</on_poweroff>
+ <on_reboot>restart</on_reboot>
+ <on_crash>destroy</on_crash>
+ <devices>
+ <emulator>/usr/bin/qemu</emulator>
+ <disk type='block' device='disk'>
+ <source dev='/dev/HostVG/QEMUGuest1'/>
+ <target dev='hda' bus='ide'/>
+ <address type='drive' controller='0' bus='0' unit='0'/>
+ </disk>
+ <disk type='network' device='disk'>
+ <driver name='qemu' type='raw'/>
+ <auth username='myname'>
+ <secret type='ceph' usage='mycluster_myname'/>
+ </auth>
+ <source protocol='rbd' name='pool/image'>
+ <host name='mon1.example.org' port='6321'/>
+ <host name='mon2.example.org' port='6322'/>
+ <host name='mon3.example.org' port='6322'/>
+ </source>
+ <target dev='vda' bus='virtio'/>
+ </disk>
+ <controller type='ide' index='0'/>
+ <memballoon model='virtio'/>
+ </devices>
+</domain>
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-ceph-env.args b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-ceph-env.args
new file mode 100644
index 0000000..3ab1219
--- /dev/null
+++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-ceph-env.args
@@ -0,0 +1,6 @@
+LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test CEPH_ARGS=-m \
+mon1.example.org:6321,mon2.example.org:6322,mon3.example.org:6322 \
+/usr/bin/qemu -S -M pc -m 214 -smp 1 -nographic -monitor \
+unix:/tmp/test-monitor,server,nowait -no-acpi -boot c -drive \
+file=/dev/HostVG/QEMUGuest1,if=ide,bus=0,unit=0 -drive file=rbd:pool/image,\
+if=virtio,format=raw -net none -serial none -parallel none -usb
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-ceph-env.xml b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-ceph-env.xml
new file mode 100644
index 0000000..e920db1
--- /dev/null
+++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-ceph-env.xml
@@ -0,0 +1,34 @@
+<domain type='qemu'>
+ <name>QEMUGuest1</name>
+ <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
+ <memory>219136</memory>
+ <currentMemory>219136</currentMemory>
+ <vcpu>1</vcpu>
+ <os>
+ <type arch='i686' machine='pc'>hvm</type>
+ <boot dev='hd'/>
+ </os>
+ <clock offset='utc'/>
+ <on_poweroff>destroy</on_poweroff>
+ <on_reboot>restart</on_reboot>
+ <on_crash>destroy</on_crash>
+ <devices>
+ <emulator>/usr/bin/qemu</emulator>
+ <disk type='block' device='disk'>
+ <source dev='/dev/HostVG/QEMUGuest1'/>
+ <target dev='hda' bus='ide'/>
+ <address type='drive' controller='0' bus='0' unit='0'/>
+ </disk>
+ <disk type='network' device='disk'>
+ <driver name='qemu' type='raw'/>
+ <source protocol='rbd' name='pool/image'>
+ <host name='mon1.example.org' port='6321'/>
+ <host name='mon2.example.org' port='6322'/>
+ <host name='mon3.example.org' port='6322'/>
+ </source>
+ <target dev='vda' bus='virtio'/>
+ </disk>
+ <controller type='ide' index='0'/>
+ <memballoon model='virtio'/>
+ </devices>
+</domain>
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd.args b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd.args
index 3ab1219..706ba89 100644
--- a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd.args
+++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd.args
@@ -1,6 +1,7 @@
-LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test CEPH_ARGS=-m \
-mon1.example.org:6321,mon2.example.org:6322,mon3.example.org:6322 \
+LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test \
/usr/bin/qemu -S -M pc -m 214 -smp 1 -nographic -monitor \
unix:/tmp/test-monitor,server,nowait -no-acpi -boot c -drive \
-file=/dev/HostVG/QEMUGuest1,if=ide,bus=0,unit=0 -drive file=rbd:pool/image,\
+file=/dev/HostVG/QEMUGuest1,if=ide,bus=0,unit=0 -drive \
+file=rbd:pool/image:\
+mon_host=mon1.example.org\:6321\;mon2.example.org\:6322\;mon3.example.org\:6322,\
if=virtio,format=raw -net none -serial none -parallel none -usb
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index 4d6db01..e7c1ffd 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -12,6 +12,7 @@
# include "internal.h"
# include "testutils.h"
+# include "util/memory.h"
# include "qemu/qemu_capabilities.h"
# include "qemu/qemu_command.h"
# include "qemu/qemu_domain.h"
@@ -23,6 +24,60 @@
static const char *abs_top_srcdir;
static struct qemud_driver driver;
+static unsigned char *
+fakeSecretGetValue(virSecretPtr obj ATTRIBUTE_UNUSED,
+ size_t *value_size,
+ unsigned int fakeflags ATTRIBUTE_UNUSED,
+ unsigned int internalFlags ATTRIBUTE_UNUSED)
+{
+ char *secret = strdup("AQCVn5hO6HzFAhAAq0NCv8jtJcIcE+HOBlMQ1A");
+ *value_size = strlen(secret);
+ return (unsigned char *) secret;
+}
+
+static virSecretPtr
+fakeSecretLookupByUsage(virConnectPtr conn,
+ int usageType ATTRIBUTE_UNUSED,
+ const char *usageID)
+{
+ virSecretPtr ret = NULL;
+ int err;
+ if (STRNEQ(usageID, "mycluster_myname"))
+ return NULL;
+ err = VIR_ALLOC(ret);
+ if (err < 0)
+ return NULL;
+ ret->magic = VIR_SECRET_MAGIC;
+ ret->refs = 1;
+ ret->usageID = strdup(usageID);
+ if (!ret->usageID)
+ return NULL;
+ ret->conn = conn;
+ conn->refs++;
+ return ret;
+}
+
+static int
+fakeSecretClose(virConnectPtr conn ATTRIBUTE_UNUSED)
+{
+ return 0;
+}
+
+static virSecretDriver fakeSecretDriver = {
+ .name = "fake_secret",
+ .open = NULL,
+ .close = fakeSecretClose,
+ .numOfSecrets = NULL,
+ .listSecrets = NULL,
+ .lookupByUUID = NULL,
+ .lookupByUsage = fakeSecretLookupByUsage,
+ .defineXML = NULL,
+ .getXMLDesc = NULL,
+ .setValue = NULL,
+ .getValue = fakeSecretGetValue,
+ .undefine = NULL,
+};
+
static int testCompareXMLToArgvFiles(const char *xml,
const char *cmdline,
virBitmapPtr extraFlags,
@@ -44,6 +99,7 @@ static int testCompareXMLToArgvFiles(const char *xml,
if (!(conn = virGetConnect()))
goto fail;
+ conn->secretDriver = &fakeSecretDriver;
len = virtTestLoadFile(cmdline, &expectargv);
if (len < 0)
@@ -357,6 +413,8 @@ mymain(void)
QEMU_CAPS_DRIVE, QEMU_CAPS_DRIVE_FORMAT);
DO_TEST("disk-drive-network-sheepdog", false,
QEMU_CAPS_DRIVE, QEMU_CAPS_DRIVE_FORMAT);
+ DO_TEST("disk-drive-network-rbd-auth", false,
+ QEMU_CAPS_DRIVE, QEMU_CAPS_DRIVE_FORMAT);
DO_TEST("disk-drive-no-boot", false,
QEMU_CAPS_DRIVE, QEMU_CAPS_DEVICE, QEMU_CAPS_BOOTINDEX);
DO_TEST("disk-usb", false, NONE);
--
1.7.1
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH v5 4/4] qemu/rbd: improve rbd device specification
2011-11-01 1:29 ` [PATCH v5 " Josh Durgin
@ 2011-11-16 0:05 ` Eric Blake
2011-11-16 1:37 ` Josh Durgin
2011-11-16 10:25 ` [libvirt] " Daniel P. Berrange
0 siblings, 2 replies; 27+ messages in thread
From: Eric Blake @ 2011-11-16 0:05 UTC (permalink / raw)
To: Josh Durgin; +Cc: libvir-list, ceph-devel
[-- Attachment #1: Type: text/plain, Size: 4079 bytes --]
On 10/31/2011 07:29 PM, Josh Durgin wrote:
> From: Sage Weil <sage@newdream.net>
Sorry for letting my review of this slip 2 weeks.
>
> This improves the support for qemu rbd devices by adding support for a few
> key features (e.g., authentication) and cleaning up the way in which
> rbd configuration options are passed to qemu.
>
> An <auth> member of the disk source xml specifies how librbd should
> authenticate. The username attribute is the Ceph/RBD user to authenticate as.
> The usage or uuid attributes specify which secret to use. Usage is an
> arbitrary identifier local to libvirt.
>
> The old RBD support relied on setting an environment variable to
> communicate information to qemu/librbd. Instead, pass those options
> explicitly to qemu. Update the qemu argument parsing and tests
> accordingly.
>
> Signed-off-by: Sage Weil <sage@newdream.net>
> Signed-off-by: Josh Durgin <josh.durgin@dreamhost.com>
> ---
>
> Changes since v4:
> * fixes memory management issues
> * keep older rbd command line parsing and test case
> * check qemuAddRBDHost return values
> * use more efficient virBuffer functions
Looks like you got all my review points.
ACK and pushed, although I do have some questions that may deserve
followup patches:
> +static int
> +qemuBuildRBDString(virConnectPtr conn,
> + virDomainDiskDefPtr disk,
> + virBufferPtr opt)
> +{
> + int i, ret = 0;
> + virSecretPtr sec = NULL;
> + char *secret = NULL;
> + size_t secret_size;
> +
> + virBufferAsprintf(opt, "rbd:%s", disk->src);
> + if (disk->auth.username) {
> + virBufferEscape(opt, ":", ":id=%s", disk->auth.username);
This results in ambiguous output if disk->auth.username can end in a
single backslash (since then, you would have \: when combined with the
next part of the option, making it look like the next ":mon_host="
option is instead a continuation of the ":id=" username). Should we be
escaping backslash as well as colon? Or should virBufferEscape be
taught to always escape backslash in addition to whatever characters
were passed in to its 'toescape' argument?
> + if (sec) {
> + char *base64 = NULL;
> +
> + secret = (char *)conn->secretDriver->getValue(sec, &secret_size, 0,
> + VIR_SECRET_GET_VALUE_INTERNAL_CALL);
> + if (secret == NULL) {
> + qemuReportError(VIR_ERR_INTERNAL_ERROR,
> + _("could not get the value of the secret for username %s"),
> + disk->auth.username);
> + goto error;
> + }
> + /* qemu/librbd wants it base64 encoded */
> + base64_encode_alloc(secret, secret_size, &base64);
> + if (!base64) {
> + virReportOOMError();
> + goto error;
> + }
> + virBufferEscape(opt, ":", ":key=%s:auth_supported=cephx none",
> + base64);
> + VIR_FREE(base64);
The command line that we pass to qemu gets logged. But what happens if
the secret was marked as ephemeral - could we be violating the premise
of not exposing passwords to too broad an audience? Or are we already
safe in that the log entries created by virCommand can only be exposed
to users that already can get at the secret information by other means?
Maybe this means we should we be adding capabilities into virCommand to
prevent the logging of the actual secret (whether base64-encoded or
otherwise), and instead log an alternate string? That is, should
virCommand be tracking parallel argv arrays; the real array passed to
exec() but never logged, and the alternate array (normally matching the
real one, but which can differ in this particular case of passing an
argument that contains a password)?
--
Eric Blake eblake@redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 620 bytes --]
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH v5 4/4] qemu/rbd: improve rbd device specification
2011-11-16 0:05 ` Eric Blake
@ 2011-11-16 1:37 ` Josh Durgin
2011-11-16 15:40 ` Eric Blake
2011-11-16 10:25 ` [libvirt] " Daniel P. Berrange
1 sibling, 1 reply; 27+ messages in thread
From: Josh Durgin @ 2011-11-16 1:37 UTC (permalink / raw)
To: Eric Blake; +Cc: libvir-list, ceph-devel
On 11/15/2011 04:05 PM, Eric Blake wrote:
> On 10/31/2011 07:29 PM, Josh Durgin wrote:
>> From: Sage Weil<sage@newdream.net>
>
> Sorry for letting my review of this slip 2 weeks.
>
>>
>> This improves the support for qemu rbd devices by adding support for a few
>> key features (e.g., authentication) and cleaning up the way in which
>> rbd configuration options are passed to qemu.
>>
>> An<auth> member of the disk source xml specifies how librbd should
>> authenticate. The username attribute is the Ceph/RBD user to authenticate as.
>> The usage or uuid attributes specify which secret to use. Usage is an
>> arbitrary identifier local to libvirt.
>>
>> The old RBD support relied on setting an environment variable to
>> communicate information to qemu/librbd. Instead, pass those options
>> explicitly to qemu. Update the qemu argument parsing and tests
>> accordingly.
>>
>> Signed-off-by: Sage Weil<sage@newdream.net>
>> Signed-off-by: Josh Durgin<josh.durgin@dreamhost.com>
>> ---
>>
>> Changes since v4:
>> * fixes memory management issues
>> * keep older rbd command line parsing and test case
>> * check qemuAddRBDHost return values
>> * use more efficient virBuffer functions
>
> Looks like you got all my review points.
>
> ACK and pushed, although I do have some questions that may deserve
> followup patches:
>
>> +static int
>> +qemuBuildRBDString(virConnectPtr conn,
>> + virDomainDiskDefPtr disk,
>> + virBufferPtr opt)
>> +{
>> + int i, ret = 0;
>> + virSecretPtr sec = NULL;
>> + char *secret = NULL;
>> + size_t secret_size;
>> +
>> + virBufferAsprintf(opt, "rbd:%s", disk->src);
>> + if (disk->auth.username) {
>> + virBufferEscape(opt, ":", ":id=%s", disk->auth.username);
>
> This results in ambiguous output if disk->auth.username can end in a
> single backslash (since then, you would have \: when combined with the
> next part of the option, making it look like the next ":mon_host="
> option is instead a continuation of the ":id=" username). Should we be
> escaping backslash as well as colon? Or should virBufferEscape be
> taught to always escape backslash in addition to whatever characters
> were passed in to its 'toescape' argument?
Escaping backslashes wouldn't hurt, but these usernames aren't expected
to have backslashes in them (they're genericNames in the xml schema).
>
>> + if (sec) {
>> + char *base64 = NULL;
>> +
>> + secret = (char *)conn->secretDriver->getValue(sec,&secret_size, 0,
>> + VIR_SECRET_GET_VALUE_INTERNAL_CALL);
>> + if (secret == NULL) {
>> + qemuReportError(VIR_ERR_INTERNAL_ERROR,
>> + _("could not get the value of the secret for username %s"),
>> + disk->auth.username);
>> + goto error;
>> + }
>> + /* qemu/librbd wants it base64 encoded */
>> + base64_encode_alloc(secret, secret_size,&base64);
>> + if (!base64) {
>> + virReportOOMError();
>> + goto error;
>> + }
>> + virBufferEscape(opt, ":", ":key=%s:auth_supported=cephx none",
>> + base64);
>> + VIR_FREE(base64);
>
> The command line that we pass to qemu gets logged. But what happens if
> the secret was marked as ephemeral - could we be violating the premise
> of not exposing passwords to too broad an audience? Or are we already
> safe in that the log entries created by virCommand can only be exposed
> to users that already can get at the secret information by other means?
The secret can be read from the command line of the running process,
which is even less secure than the log. I'm working on passing the
secret via the qemu monitor instead of the command line, which will
avoid both issues.
> Maybe this means we should we be adding capabilities into virCommand to
> prevent the logging of the actual secret (whether base64-encoded or
> otherwise), and instead log an alternate string? That is, should
> virCommand be tracking parallel argv arrays; the real array passed to
> exec() but never logged, and the alternate array (normally matching the
> real one, but which can differ in this particular case of passing an
> argument that contains a password)?
>
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH v5 4/4] qemu/rbd: improve rbd device specification
2011-11-16 1:37 ` Josh Durgin
@ 2011-11-16 15:40 ` Eric Blake
0 siblings, 0 replies; 27+ messages in thread
From: Eric Blake @ 2011-11-16 15:40 UTC (permalink / raw)
To: Josh Durgin; +Cc: libvir-list, ceph-devel
[-- Attachment #1: Type: text/plain, Size: 1502 bytes --]
On 11/15/2011 06:37 PM, Josh Durgin wrote:
>> The command line that we pass to qemu gets logged. But what happens if
>> the secret was marked as ephemeral - could we be violating the premise
>> of not exposing passwords to too broad an audience? Or are we already
>> safe in that the log entries created by virCommand can only be exposed
>> to users that already can get at the secret information by other means?
>
> The secret can be read from the command line of the running process,
> which is even less secure than the log. I'm working on passing the
> secret via the qemu monitor instead of the command line, which will
> avoid both issues.
>
>> Maybe this means we should we be adding capabilities into virCommand to
>> prevent the logging of the actual secret (whether base64-encoded or
>> otherwise), and instead log an alternate string? That is, should
>> virCommand be tracking parallel argv arrays; the real array passed to
>> exec() but never logged, and the alternate array (normally matching the
>> real one, but which can differ in this particular case of passing an
>> argument that contains a password)?
Given your arguments (that ps can read argv of qemu, even if we hid it
from libvirt logs, and that we will be moving to a monitor command as
soon as qemu supports one), I see no reason to hack up virCommand to
support alternate log output.
--
Eric Blake eblake@redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 620 bytes --]
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [libvirt] [PATCH v5 4/4] qemu/rbd: improve rbd device specification
2011-11-16 0:05 ` Eric Blake
2011-11-16 1:37 ` Josh Durgin
@ 2011-11-16 10:25 ` Daniel P. Berrange
1 sibling, 0 replies; 27+ messages in thread
From: Daniel P. Berrange @ 2011-11-16 10:25 UTC (permalink / raw)
To: Eric Blake; +Cc: Josh Durgin, libvir-list, ceph-devel
On Tue, Nov 15, 2011 at 05:05:27PM -0700, Eric Blake wrote:
> On 10/31/2011 07:29 PM, Josh Durgin wrote:
> > From: Sage Weil <sage@newdream.net>
> > + if (sec) {
> > + char *base64 = NULL;
> > +
> > + secret = (char *)conn->secretDriver->getValue(sec, &secret_size, 0,
> > + VIR_SECRET_GET_VALUE_INTERNAL_CALL);
> > + if (secret == NULL) {
> > + qemuReportError(VIR_ERR_INTERNAL_ERROR,
> > + _("could not get the value of the secret for username %s"),
> > + disk->auth.username);
> > + goto error;
> > + }
> > + /* qemu/librbd wants it base64 encoded */
> > + base64_encode_alloc(secret, secret_size, &base64);
> > + if (!base64) {
> > + virReportOOMError();
> > + goto error;
> > + }
> > + virBufferEscape(opt, ":", ":key=%s:auth_supported=cephx none",
> > + base64);
> > + VIR_FREE(base64);
>
> The command line that we pass to qemu gets logged. But what happens if
> the secret was marked as ephemeral - could we be violating the premise
> of not exposing passwords to too broad an audience? Or are we already
> safe in that the log entries created by virCommand can only be exposed
> to users that already can get at the secret information by other means?
>
> Maybe this means we should we be adding capabilities into virCommand to
> prevent the logging of the actual secret (whether base64-encoded or
> otherwise), and instead log an alternate string? That is, should
> virCommand be tracking parallel argv arrays; the real array passed to
> exec() but never logged, and the alternate array (normally matching the
> real one, but which can differ in this particular case of passing an
> argument that contains a password)?
The passing of secrets on the command line is just a temporary hack
we're doing to prove the overall handling of passwords for Ceph. The
real plan is to set them via monitor command in QEMU, but we're just
waiting for some QEMU work before changing libvirt todo that.
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [RFC PATCH v3 0/4] Improve Ceph Qemu+RBD support
2011-10-20 18:01 [RFC PATCH v3 0/4] Improve Ceph Qemu+RBD support Josh Durgin
` (3 preceding siblings ...)
2011-10-20 18:01 ` [RFC PATCH v3 4/4] qemu/rbd: improve rbd device specification Josh Durgin
@ 2011-10-27 5:19 ` Sage Weil
4 siblings, 0 replies; 27+ messages in thread
From: Sage Weil @ 2011-10-27 5:19 UTC (permalink / raw)
To: berrange; +Cc: Josh Durgin, libvir-list, ceph-devel
Hi Daniel,
Is this iteration closer to what you had in mind?
Obscuring the passing of secrets into qemu is going to need changes on the
qemu end, but it would be great to get authentication at least working in
the meantime.
sage
On Thu, 20 Oct 2011, Josh Durgin 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 patch passes virConnectPtr into the Domain{Attach,Detach}
> methods (needed to access secrets while building the qemu command).
>
> 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 (there are a few ways to do this, which will be discussed
> in a separate email).
>
> Changes from v2:
> make <auth> a direct child of <disk> instead of <source>
> allow secret lookup by UUID or usage
> test with fake secret driver
> other fixes from Daniel's review
>
> Changes from v1:
> update docs/schemas/{domain,secret}.rng
>
> Josh Durgin (1):
> storage: add auth to virDomainDiskDef
>
> Sage Weil (3):
> secret: add Ceph secret type
> qemu: pass virConnectPtr into Domain{Attach,Detach}*
> qemu/rbd: improve rbd device specification
>
> docs/schemas/domaincommon.rng | 29 ++
> docs/schemas/secret.rng | 10 +
> include/libvirt/libvirt.h.in | 3 +
> src/Makefile.am | 3 +-
> src/conf/domain_conf.c | 105 +++++++-
> src/conf/domain_conf.h | 17 ++
> src/conf/secret_conf.c | 23 ++-
> src/conf/secret_conf.h | 1 +
> src/qemu/qemu_command.c | 289 ++++++++++++--------
> 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 +
> .../qemuxml2argv-disk-drive-network-rbd-auth.args | 6 +
> .../qemuxml2argv-disk-drive-network-rbd-auth.xml | 37 +++
> .../qemuxml2argv-disk-drive-network-rbd.args | 6 +-
> tests/qemuxml2argvtest.c | 52 ++++
> 18 files changed, 485 insertions(+), 148 deletions(-)
> create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-auth.args
> create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-auth.xml
>
> --
> 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] 27+ messages in thread