[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH 11/20] Add volume encryption information handling.



On Tue, Aug 04, 2009 at 10:28:26PM +0200, Miloslav Trma?? wrote:
> Define an <encryption> tag specifying volume encryption format and
> format-depenedent parameters (e.g. passphrase, cipher name, key
> length, key).
> 
> Currently the only defined parameter is a reference to a "secret"
> (passphrase/key) managed using the virSecret* API.
> 
> Only the qcow/qcow2 encryption format, and a "default" format used to
> let libvirt choose the format during volume creation, is currently
> supported.
> 
> This patch does not add any users; the <encryption> tag is added in
> the following patches to both volumes (to support encrypted volume
> creation) and domains.
> 
> Changes since the first submission;
> - Use <secret type='passphrase' secret_id='...'>
>   instead of <passphrase> with in-line passphrase.
> - Use a generic "sequence of secrets" representation.
> - Output the <secret> elements unconditionally (they don't reveal the
>   secrets any more).
> - Add format "default", to be used during volume creation only.
> - Use "%s", _("...") for all error messages without parameters.
> - Add a schema for <encryption>.
> - Document <encryption>.


> diff --git a/docs/schemas/storageencryption.rng b/docs/schemas/storageencryption.rng
> new file mode 100644
> index 0000000..69a2841
> --- /dev/null
> +++ b/docs/schemas/storageencryption.rng
> @@ -0,0 +1,37 @@
> +<!-- A Relax NG schema for the libvirt volume encryption XML format -->
> +<grammar xmlns="http://relaxng.org/ns/structure/1.0";
> +    datatypeLibrary="http://www.w3.org/2001/XMLSchema-datatypes";>
> +
> +  <define name='encryption'>
> +    <optional>
> +      <element name='encryption'>
> +	<attribute name='format'>
> +	  <choice>
> +	    <value>unencrypted</value>
> +	    <value>default</value>
> +	    <value>qcow</value>
> +	  </choice>
> +	</attribute>

I don't think we should include 'unencrypted' here. If a volume is
not encrypted, we should simply omit the <encryption> element
entirely in the domain  / storage volume XML doc.

> +	<zeroOrMore>
> +	  <ref name='secret'/>
> +	</zeroOrMore>
> +      </element>
> +    </optional>
> +  </define>
> +
> +  <define name='secret'>
> +    <element name='secret'>
> +      <attribute name='type'>
> +	<choice>
> +	  <value>passphrase</value>
> +	</choice>
> +      </attribute>
> +      <optional>
> +        <attribute name='secret_id'>
> +          <text/>
> +        </attribute>

Lets just call this attribute  'uuid' - no need to have 
the word 'secret' prefixed on it too.


> +      </optional>
> +    </element>
> +  </define>
> +
> +</grammar>


> +static int
> +virStorageEncryptionSecretFormat(virConnectPtr conn,
> +                                 virBufferPtr buf,
> +                                 virStorageEncryptionSecretPtr secret)
> +{
> +    const char *type;
> +
> +    type = virStorageEncryptionSecretTypeTypeToString(secret->type);
> +    if (!type) {
> +        virStorageReportError(conn, VIR_ERR_INTERNAL_ERROR, "%s",
> +                              _("unexpected volume encryption secret type"));
> +        return -1;
> +    }
> +
> +    virBufferVSprintf(buf, "    <secret type='%s'", type);
> +    if (secret->secret_id != NULL)
> +      virBufferEscapeString(buf, " secret_id='%s'", secret->secret_id);
> +    virBufferAddLit(buf, "/>\n");
> +    return 0;
> +}

Tiny indentation bug crept in there.

> +
> +int
> +virStorageEncryptionFormat(virConnectPtr conn,
> +                           virBufferPtr buf,
> +                           virStorageEncryptionPtr enc)
> +{
> +    const char *format;
> +    size_t i;
> +
> +    format = virStorageEncryptionFormatTypeToString(enc->format);
> +    if (!format) {
> +        virStorageReportError(conn, VIR_ERR_INTERNAL_ERROR,
> +                              "%s", _("unexpected encryption format"));
> +        return -1;
> +    }
> +    virBufferVSprintf(buf, "  <encryption format='%s'>\n", format);
> +
> +    for (i = 0; i < enc->nsecrets; i++) {
> +      if (virStorageEncryptionSecretFormat(conn, buf, enc->secrets[i]) < 0)
> +        return -1;
> +    }


And there too.

> +
> +    virBufferAddLit(buf, "  </encryption>\n");
> +
> +    return 0;
> +}



I think this is generally ok, aside from those minor stylist changes.

Daniel
-- 
|: Red Hat, Engineering, London   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]