Re: [Qemu-devel] [PATCH v2 10/17] block: add generic full disk encryption driver

From: Fam Zheng <famz@redhat.com>
To: "Daniel P. Berrange" <berrange@redhat.com>
Cc: Kevin Wolf <kwolf@redhat.com>,
	qemu-devel@nongnu.org, qemu-block@nongnu.org
Subject: Re: [Qemu-devel] [PATCH v2 10/17] block: add generic full disk encryption driver
Date: Thu, 21 Jan 2016 21:01:19 +0800	[thread overview]
Message-ID: <20160121130119.GA22797@ad.usersys.redhat.com> (raw)
In-Reply-To: <20160121110230.GI19835@redhat.com>

On Thu, 01/21 11:02, Daniel P. Berrange wrote:
> On Thu, Jan 21, 2016 at 05:12:08PM +0800, Fam Zheng wrote:
> > On Wed, 01/20 17:38, Daniel P. Berrange wrote:
> > > +    /* XXX Should we treat size as being total physical size
> > > +     * of the image (ie payload + encryption header), or just
> > > +     * the logical size of the image (ie payload). If the latter
> > > +     * then we need to extend 'size' to include the header
> > > +     * size */
> > 
> > The latter. :)
> 
> Ok
> 
> > > +    qemu_opt_set_number(opts, BLOCK_OPT_SIZE, size, &error_abort);
> > > +#define BLOCK_CRYPTO_DRIVER(name, format)                               \
> > > +    static int block_crypto_probe_ ## name(const uint8_t *buf,          \
> > > +                                           int buf_size,                \
> > > +                                           const char *filename) {      \
> > > +        return block_crypto_probe_generic(format,                       \
> > > +                                          buf, buf_size, filename);     \
> > > +    }                                                                   \
> > > +                                                                        \
> > > +    static int block_crypto_open_ ## name(BlockDriverState *bs,         \
> > > +                                          QDict *options,               \
> > > +                                          int flags,                    \
> > > +                                          Error **errp)                 \
> > > +    {                                                                   \
> > > +        return block_crypto_open_generic(format,                        \
> > > +                                         &block_crypto_runtime_opts_ ## name, \
> > > +                                         bs, options, flags, errp);     \
> > > +    }                                                                   \
> > > +                                                                        \
> > > +    static int block_crypto_create_ ## name(const char *filename,       \
> > > +                                            QemuOpts *opts,             \
> > > +                                            Error **errp)               \
> > > +    {                                                                   \
> > > +        return block_crypto_create_generic(format,                      \
> > > +                                           filename, opts, errp);       \
> > > +    }                                                                   \
> > > +                                                                        \
> > > +    BlockDriver bdrv_crypto_ ## name = {                                \
> > > +        .format_name        = #name,                                    \
> > > +        .instance_size      = sizeof(BlockCrypto),                      \
> > > +        .bdrv_probe         = block_crypto_probe_ ## name,              \
> > > +        .bdrv_open          = block_crypto_open_ ## name,               \
> > > +        .bdrv_close         = block_crypto_close,                       \
> > > +        .bdrv_create        = block_crypto_create_ ## name,             \
> > > +        .create_opts        = &block_crypto_create_opts_ ## name,       \
> > > +                                                                        \
> > > +        .bdrv_co_readv      = block_crypto_co_readv,                    \
> > > +        .bdrv_co_writev     = block_crypto_co_writev,                   \
> > > +        .bdrv_getlength     = block_crypto_getlength,                   \
> > > +    }
> > > +
> > > +BLOCK_CRYPTO_DRIVER(luks, Q_CRYPTO_BLOCK_FORMAT_LUKS);
> > 
> > Personally I really prefer a preprocessed version, for the ease of grep.
> 
> I'm not sure I understand what you mean by a preprocessed version - could
> you expand on that.

I mean don't use macro concatenation and use plain symbols like in other block
drivers.

    BlockDriver bdrv_crypto_luks = {
        .format_name        = "luks",
        .instance_size      = sizeof(BlockCrypto),
        .bdrv_probe         = block_crypto_probe_luks,
        .bdrv_open          = block_crypto_open_luks,
        ...

mostly because it's easier to grep (or for refactoring with tools).

But I can't how repeatitive this would be (I can see the "don't repeat
yourself" with your approach).  There is only one BLOCK_CRYPTO_DRIVER instance
in this series. This is probably bikeshedding.

Fam