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
signature.asc
Description: OpenPGP digital signature
