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