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

[Daniel P. Berrange]

On Thu, Jan 21, 2016 at 09:01:19PM +0800, Fam Zheng wrote:
> 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.

Yeah, thinking about it, my macro approach is probably overkill for the
forseeable future anyway. I was future proofing wrt possible addition
of trucrypt support, but that's soo far off its not something i really
need worry about now.

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|