1:53 p.m.
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(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org