Re: [Qemu-devel] [PATCH] quorum: Only compile when supported

["Daniel P. Berrange" <berrange@redhat.com>]

On Tue, Jul 05, 2016 at 04:57:58PM +0800, Fam Zheng wrote:
> On Tue, 07/05 09:45, Daniel P. Berrange wrote:
> > On Tue, Jun 28, 2016 at 09:47:47AM +0800, Fam Zheng wrote:
> > > This was the only exceptional module init function that does something
> > > else than a simple list of bdrv_register() calls, in all the block
> > > drivers.
> > > 
> > > The qcrypto_hash_supports is actually a static check, determined at
> > > compile time.  Follow the block-job-$(CONFIG_FOO) convention for
> > > consistency.
> > > 
> > > Signed-off-by: Fam Zheng <famz@redhat.com>
> > 
> > The point of using qcrypto_hash_supports() is that it isolates the
> > block code Makefile rules from the details of the current specific
> > impl of the hash APIs in QEMU. As a prime example of why this is
> > important, try rebasing to GIT master, and you'll find we no longer
> > use gnutls for the hash APIs. We choose between libgcrypt, nettle
> > or a empty stub for hash impls now. I think it is a backwards step
> > to add back these makefile conditionals
> 
> I don't want to spoil the isolation, but I think it's also worth to keep the
> reasoning of whether a driver is supported as simple as possible. In other
> words, if it's determined at configure time, there is no reason to use a
> runtime check, right?

While the crypto algorithms may all be built-in at compile time, there can
be situations where they are forbidden from use at runtime. For example
the FIPS mode activation will disable many algorithms from use. You might
say that this doesn't matter since FIPs mode dosn't affect SHA256, but the
point of having the generic crypto API inside QEMU is to stop us sprinkling
these kind of ad-hoc assumptions about crypto policies across the code base.

Can you backup and explain more detail what the actual problem you're trying
to solve is. IIUC, it is related to module loading, but I'm not seeing exactly
what it is. Surely when we load the quorum.so module, we'll just invoke the
bdrv_quorum_init() method as normal, so I would have expected the current
logic to continue to "just work".  ie, just because we load a module, does
not mean that module should be required to register its block driver.

The other alternative though is to simply remove the hash check from the
init method *and* unconditionally compile it, and simply allow the
quorum_open() method do the qcrypto_hash_supports() check. This would
be the same way that the LUKS block driver works - it has many crypto
algorithms in use, chosen dynamically, so it has no choice but to test
this at open() time.

> > > diff --git a/block/quorum.c b/block/quorum.c
> > > index 331b726..18fbed8 100644
> > > --- a/block/quorum.c
> > > +++ b/block/quorum.c
> > > @@ -1113,10 +1113,6 @@ static BlockDriver bdrv_quorum = {
> > >  
> > >  static void bdrv_quorum_init(void)
> > >  {
> > > -    if (!qcrypto_hash_supports(QCRYPTO_HASH_ALG_SHA256)) {
> > > -        /* SHA256 hash support is required for quorum device */
> > > -        return;
> > > -    }
> > >      bdrv_register(&bdrv_quorum);
> > >  }

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 :|