Re: [Qemu-devel] [PATCH v2 06/17] crypto: add block encryption framework

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3091 bytes --]

On 02/05/2016 05:43 AM, Daniel P. Berrange wrote:
> On Thu, Feb 04, 2016 at 05:23:32PM -0700, Eric Blake wrote:
>> On 01/20/2016 10:38 AM, Daniel P. Berrange wrote:
>>> Add a generic framework for support different block encryption
>>> formats. Upon instantiating a QCryptoBlock object, it will read
>>> the encryption header and extract the encryption keys. It is
>>> then possible to call methods to encrypt/decrypt data buffers.
>>>
>>> There is also a mode whereby it will create/initialize a new
>>> encryption header on a previously unformatted volume.
>>>
>>> The initial framework comes with support for the legacy QCow
>>> AES based encryption. This enables code in the QCow driver to
>>> be consolidated later.
>>>

> 
>>> +static gboolean
>>> +qcrypto_block_qcow_has_format(const uint8_t *buf G_GNUC_UNUSED,
>>> +                              size_t buf_size G_GNUC_UNUSED)
>>> +{
>>> +    return false;
>>> +}
>>
>> When I see gboolean, I think TRUE/FALSE.  Yes, C99 'false' happens to
>> promote to the correct value for whatever integer type gboolean is, but
>> this would read nicer if it returned 'bool'.
> 
> Yeah, dunno what I was thinking really. I use gboolean in a few places
> due to the gmainloop callback API contract, but this should not have
> needed it.

To be honest, when I see 'gboolean', I _really_ think "Eww. Why did glib
have to botch it?".  Gnulib's <stdbool.h> replacement that gets C99
semantics in all but a few corner cases with a C89 compiler is so much
nicer than glib's blatant wrong typing.


>>> +
>>> +    len = strlen(password);
>>> +    if (len > 16) {
>>> +        len = 16;
>>> +    }
>>> +    for (i = 0; i < len; i++) {
>>> +        keybuf[i] = password[i];
>>> +    }
>>
>> What - we really throw away anything longer than 16 bytes?
> 
> Yes, that's how awesome legacy qcow2 encryption is :-)

I should have mentioned that I saw nothing with this part of your patch,
and was just vocalizing my disgust at what code we are maintaining.  But
it looks like you got my meaning :)


>>> +
>>> +struct QCryptoBlockDriver {
>>> +    int (*open)(QCryptoBlock *block,
>>> +                QCryptoBlockOpenOptions *options,
>>> +                QCryptoBlockReadFunc readfunc,
>>> +                void *opaque,
>>> +                unsigned int flags,
>>> +                Error **errp);
>>
>> No documentation on any of these contracts?
> 
> They're essentially identical to the public API contracts, so I
> figured it was redundant to duplicate those docs.

Fair enough.  Although I probably could have been spared some confusion
if you had done:

git config diff.orderFile /path/to/foo

the populated foo such that include/* comes before any other file.  I
recently learned that trick; it's also available as git format-patch
-O/path/to/file (with no space after -O), and can make patches a lot
easier to read when you focus the reader on interface first.

-- 
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: 604 bytes --]