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

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

[Fam Zheng]

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>
---
 block/Makefile.objs | 2 +-
 block/quorum.c      | 4 ----
 2 files changed, 1 insertion(+), 5 deletions(-)

diff --git a/block/Makefile.objs b/block/Makefile.objs
index 44a5416..c87d605 100644
--- a/block/Makefile.objs
+++ b/block/Makefile.objs
@@ -3,7 +3,7 @@ block-obj-y += qcow2.o qcow2-refcount.o qcow2-cluster.o qcow2-snapshot.o qcow2-c
 block-obj-y += qed.o qed-gencb.o qed-l2-cache.o qed-table.o qed-cluster.o
 block-obj-y += qed-check.o
 block-obj-$(CONFIG_VHDX) += vhdx.o vhdx-endian.o vhdx-log.o
-block-obj-y += quorum.o
+block-obj-$(CONFIG_GNUTLS_HASH) += quorum.o
 block-obj-y += parallels.o blkdebug.o blkverify.o blkreplay.o
 block-obj-y += block-backend.o snapshot.o qapi.o
 block-obj-$(CONFIG_WIN32) += raw-win32.o win32-aio.o
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);
 }
 
-- 
2.9.0

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

[Alberto Garcia]

On Tue 28 Jun 2016 03:47:47 AM CEST, Fam Zheng <famz@redhat.com> wrote:

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

Oh, so we didn't see this in e94867e :-)

Reviewed-by: Alberto Garcia <berto@igalia.com>

Berto

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

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 2196 bytes --]

On 28.06.2016 03:47, 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.

This sounds like this patch specifically wants to drop the check from
bdrv_quorum_init().

I think keeping the check would be better; while we can probably assume
that SHA256 is supported if CONFIG_GNUTLS_HASH is defined, it would
still technically be better to actually check for that support.

However, since this is probably only a theoretical issue, if there's a
good reason (like preparation for dynamic block driver modules) for
having to drop the check from bdrv_quorum_init(), I think it's alright
to do so.

Max

> 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>
> ---
>  block/Makefile.objs | 2 +-
>  block/quorum.c      | 4 ----
>  2 files changed, 1 insertion(+), 5 deletions(-)
> 
> diff --git a/block/Makefile.objs b/block/Makefile.objs
> index 44a5416..c87d605 100644
> --- a/block/Makefile.objs
> +++ b/block/Makefile.objs
> @@ -3,7 +3,7 @@ block-obj-y += qcow2.o qcow2-refcount.o qcow2-cluster.o qcow2-snapshot.o qcow2-c
>  block-obj-y += qed.o qed-gencb.o qed-l2-cache.o qed-table.o qed-cluster.o
>  block-obj-y += qed-check.o
>  block-obj-$(CONFIG_VHDX) += vhdx.o vhdx-endian.o vhdx-log.o
> -block-obj-y += quorum.o
> +block-obj-$(CONFIG_GNUTLS_HASH) += quorum.o
>  block-obj-y += parallels.o blkdebug.o blkverify.o blkreplay.o
>  block-obj-y += block-backend.o snapshot.o qapi.o
>  block-obj-$(CONFIG_WIN32) += raw-win32.o win32-aio.o
> 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);
>  }
>  
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 498 bytes --]

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

[Alberto Garcia]

On Sat 02 Jul 2016 02:36:38 PM CEST, Max Reitz 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.
>
> This sounds like this patch specifically wants to drop the check from
> bdrv_quorum_init().
>
> I think keeping the check would be better; while we can probably
> assume that SHA256 is supported if CONFIG_GNUTLS_HASH is defined, it
> would still technically be better to actually check for that support.

Right, but SHA256 was added to GNUTLS almost 10 years ago, so I don't
think that adding such check has any practical effect.

Berto

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

[Fam Zheng]

On Sat, 07/02 14:36, Max Reitz wrote:
> On 28.06.2016 03:47, 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.
> 
> This sounds like this patch specifically wants to drop the check from
> bdrv_quorum_init().
> 
> I think keeping the check would be better; while we can probably assume
> that SHA256 is supported if CONFIG_GNUTLS_HASH is defined, it would
> still technically be better to actually check for that support.

This one is effectively a static check so I think it's practically okay to skip
it, and yes, the idea is this makes it easier to do dynamic driver loading.

Thanks, 
Fam

> 
> However, since this is probably only a theoretical issue, if there's a
> good reason (like preparation for dynamic block driver modules) for
> having to drop the check from bdrv_quorum_init(), I think it's alright
> to do so.
> 
> Max
> 
> > 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>
> > ---
> >  block/Makefile.objs | 2 +-
> >  block/quorum.c      | 4 ----
> >  2 files changed, 1 insertion(+), 5 deletions(-)
> > 
> > diff --git a/block/Makefile.objs b/block/Makefile.objs
> > index 44a5416..c87d605 100644
> > --- a/block/Makefile.objs
> > +++ b/block/Makefile.objs
> > @@ -3,7 +3,7 @@ block-obj-y += qcow2.o qcow2-refcount.o qcow2-cluster.o qcow2-snapshot.o qcow2-c
> >  block-obj-y += qed.o qed-gencb.o qed-l2-cache.o qed-table.o qed-cluster.o
> >  block-obj-y += qed-check.o
> >  block-obj-$(CONFIG_VHDX) += vhdx.o vhdx-endian.o vhdx-log.o
> > -block-obj-y += quorum.o
> > +block-obj-$(CONFIG_GNUTLS_HASH) += quorum.o
> >  block-obj-y += parallels.o blkdebug.o blkverify.o blkreplay.o
> >  block-obj-y += block-backend.o snapshot.o qapi.o
> >  block-obj-$(CONFIG_WIN32) += raw-win32.o win32-aio.o
> > 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);
> >  }
> >  
> > 
> 
> 

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

[Sascha Silbe]

Dear Fam (or Zheng?),

Fam Zheng <famz@redhat.com> writes:

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

Good idea.


[block/Makefile.objs]
> @@ -3,7 +3,7 @@ block-obj-y += qcow2.o qcow2-refcount.o qcow2-cluster.o qcow2-snapshot.o qcow2-c
>  block-obj-y += qed.o qed-gencb.o qed-l2-cache.o qed-table.o qed-cluster.o
>  block-obj-y += qed-check.o
>  block-obj-$(CONFIG_VHDX) += vhdx.o vhdx-endian.o vhdx-log.o
> -block-obj-y += quorum.o
> +block-obj-$(CONFIG_GNUTLS_HASH) += quorum.o
[...]
[block/quorum.c]
>  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);

The quorum driver needs SHA256 which was introduced in gnutls
2.11.1. However configure sets CONFIG_GNUTLS_HASH when gnutls 2.9.10+ is
present. You should either bump the version in configure or add an
explicit configure check for SHA256.

Sascha
-- 
Softwareentwicklung Sascha Silbe, Niederhofenstraße 5/1, 71229 Leonberg
https://se-silbe.de/
USt-IdNr. DE281696641

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

[Fam Zheng]

On Tue, 07/05 09:58, Sascha Silbe wrote:
> Dear Fam (or Zheng?),

Hi Sascha,

Zheng is the last name here. :)

> 
> Fam Zheng <famz@redhat.com> writes:
> 
> > 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.
> 
> Good idea.
> 
> 
> [block/Makefile.objs]
> > @@ -3,7 +3,7 @@ block-obj-y += qcow2.o qcow2-refcount.o qcow2-cluster.o qcow2-snapshot.o qcow2-c
> >  block-obj-y += qed.o qed-gencb.o qed-l2-cache.o qed-table.o qed-cluster.o
> >  block-obj-y += qed-check.o
> >  block-obj-$(CONFIG_VHDX) += vhdx.o vhdx-endian.o vhdx-log.o
> > -block-obj-y += quorum.o
> > +block-obj-$(CONFIG_GNUTLS_HASH) += quorum.o
> [...]
> [block/quorum.c]
> >  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);
> 
> The quorum driver needs SHA256 which was introduced in gnutls
> 2.11.1. However configure sets CONFIG_GNUTLS_HASH when gnutls 2.9.10+ is
> present. You should either bump the version in configure or add an
> explicit configure check for SHA256.

Yes, I just noticed commit 0c16c056a4f removed CONFIG_GNUTLS_HASH so I need to
rebase anyway (that commit also fixed this version requirement we have been
missing as you mentioned). Thanks for reviewing!

Fam

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

[Alberto Garcia]

On Tue 05 Jul 2016 09:58:25 AM CEST, Sascha Silbe wrote:

> The quorum driver needs SHA256 which was introduced in gnutls 2.11.1.

Are you sure about that?

* Version 1.7.4 (released 2007-02-05)

[...]

** API and ABI modifications:
GNUTLS_MAC_SHA256,
GNUTLS_MAC_SHA384,
GNUTLS_MAC_SHA512: New gnutls_mac_algorithm_t values.
GNUTLS_DIG_SHA256,
GNUTLS_DIG_SHA384,
GNUTLS_DIG_SHA512: New gnutls_digest_algorithm_t values.
GNUTLS_SIGN_RSA_SHA256,
GNUTLS_SIGN_RSA_SHA384,
GNUTLS_SIGN_RSA_SHA512: New gnutls_sign_algorithm_t values.

https://gitlab.com/gnutls/gnutls/blob/master/NEWS#L6570

Berto

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

[Sascha Silbe]

Dear Alberto,

Alberto Garcia <berto@igalia.com> writes:

> On Tue 05 Jul 2016 09:58:25 AM CEST, Sascha Silbe wrote:
>
>> The quorum driver needs SHA256 which was introduced in gnutls 2.11.1.
>
> Are you sure about that?
>
> * Version 1.7.4 (released 2007-02-05)
>
> [...]
>
> ** API and ABI modifications:
> GNUTLS_MAC_SHA256,
[...]

You are right, I misinterpreted the gnutls 2.11.1 NEWS entry. It added
the RSA_NULL_SHA256 cipher suite, not the SHA256 hash algorithm.

So the existing configure check is good enough, no need to bump the
required gnutls version.

Sascha
-- 
Softwareentwicklung Sascha Silbe, Niederhofenstraße 5/1, 71229 Leonberg
https://se-silbe.de/
USt-IdNr. DE281696641

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

[Daniel P. Berrange]

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

> ---
>  block/Makefile.objs | 2 +-
>  block/quorum.c      | 4 ----
>  2 files changed, 1 insertion(+), 5 deletions(-)
> 
> diff --git a/block/Makefile.objs b/block/Makefile.objs
> index 44a5416..c87d605 100644
> --- a/block/Makefile.objs
> +++ b/block/Makefile.objs
> @@ -3,7 +3,7 @@ block-obj-y += qcow2.o qcow2-refcount.o qcow2-cluster.o qcow2-snapshot.o qcow2-c
>  block-obj-y += qed.o qed-gencb.o qed-l2-cache.o qed-table.o qed-cluster.o
>  block-obj-y += qed-check.o
>  block-obj-$(CONFIG_VHDX) += vhdx.o vhdx-endian.o vhdx-log.o
> -block-obj-y += quorum.o
> +block-obj-$(CONFIG_GNUTLS_HASH) += quorum.o
>  block-obj-y += parallels.o blkdebug.o blkverify.o blkreplay.o
>  block-obj-y += block-backend.o snapshot.o qapi.o
>  block-obj-$(CONFIG_WIN32) += raw-win32.o win32-aio.o
> 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);
>  }
>  
> -- 
> 2.9.0
> 
> 

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

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

[Fam Zheng]

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?

Fam

> 
> > ---
> >  block/Makefile.objs | 2 +-
> >  block/quorum.c      | 4 ----
> >  2 files changed, 1 insertion(+), 5 deletions(-)
> > 
> > diff --git a/block/Makefile.objs b/block/Makefile.objs
> > index 44a5416..c87d605 100644
> > --- a/block/Makefile.objs
> > +++ b/block/Makefile.objs
> > @@ -3,7 +3,7 @@ block-obj-y += qcow2.o qcow2-refcount.o qcow2-cluster.o qcow2-snapshot.o qcow2-c
> >  block-obj-y += qed.o qed-gencb.o qed-l2-cache.o qed-table.o qed-cluster.o
> >  block-obj-y += qed-check.o
> >  block-obj-$(CONFIG_VHDX) += vhdx.o vhdx-endian.o vhdx-log.o
> > -block-obj-y += quorum.o
> > +block-obj-$(CONFIG_GNUTLS_HASH) += quorum.o
> >  block-obj-y += parallels.o blkdebug.o blkverify.o blkreplay.o
> >  block-obj-y += block-backend.o snapshot.o qapi.o
> >  block-obj-$(CONFIG_WIN32) += raw-win32.o win32-aio.o
> > 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);
> >  }
> >  
> > -- 
> > 2.9.0
> > 
> > 
> 
> 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 :|

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

[Daniel P. Berrange]

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

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

[Fam Zheng]

On Tue, 07/05 10:35, Daniel P. Berrange wrote:
> 
> 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.

This patch originated when I was researching all drivers' block_init functions
about module loading, and realized this is the only one "runtime check" and is
poorly established as it's actually "static". The unnecessity is more of the
reason behind the patch.

Its relationship with module loading is, it makes implementing this idea
easier:

https://lists.gnu.org/archive/html/qemu-devel/2016-06/msg07580.html

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

You are right, module init function can do anything it wants, as long as we
really need it to. It's just not clear to me quorum is the case.

BTW thanks for sending the reverting series!

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

This sounds good too.

Fam

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

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

[Alberto Garcia]

On Tue 05 Jul 2016 10:45:21 AM CEST, Daniel P. Berrange wrote:

> 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

Now that you mention this I wonder why we are not using glib for the
hashing functions. GChecksum is available since glib 2.16 (QEMU requires
2.22) and it supports MD5, SHA1, SHA256 and SHA512. I see that in git
master there's now a few algorithms more, but for the Quorum case those
ones are enough.

Berto

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

[Daniel P. Berrange]

On Tue, Jul 05, 2016 at 11:18:29AM +0200, Alberto Garcia wrote:
> On Tue 05 Jul 2016 10:45:21 AM CEST, Daniel P. Berrange wrote:
> 
> > 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
> 
> Now that you mention this I wonder why we are not using glib for the
> hashing functions. GChecksum is available since glib 2.16 (QEMU requires
> 2.22) and it supports MD5, SHA1, SHA256 and SHA512. I see that in git
> master there's now a few algorithms more, but for the Quorum case those
> ones are enough.

The GChecksum API is inadequate for QEMU's needs, due to its limited
range of algorithms. We absolutely do not want different areas of
the code using different APIs either. The goal of the crypto APIs is
to provide a standard internal API for all cryptographic related
operations for use across the whole codebase. This has clarified much
of our code by removing countless #ifdef conditionals from the code
and similar from the build system. It also facilitates people auditing
QEMU use & implementation of crypto as there is only one place to look
at to review. It also ensures that QEMU is only using certified secure
crypto libraries, not some custom re-implementation of the crypto
algorithms that have never been through a security review. Finally is
ensures that QEMU correctly responds to runtime configurable changes,
such as FIPS mode which restricts use of certain crypto algorithms
at runtime, even if they're technically available at compile time.


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

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

[Daniel P. Berrange]

On Tue, Jul 05, 2016 at 10:26:56AM +0100, Daniel P. Berrange wrote:
> On Tue, Jul 05, 2016 at 11:18:29AM +0200, Alberto Garcia wrote:
> > On Tue 05 Jul 2016 10:45:21 AM CEST, Daniel P. Berrange wrote:
> > 
> > > 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
> > 
> > Now that you mention this I wonder why we are not using glib for the
> > hashing functions. GChecksum is available since glib 2.16 (QEMU requires
> > 2.22) and it supports MD5, SHA1, SHA256 and SHA512. I see that in git
> > master there's now a few algorithms more, but for the Quorum case those
> > ones are enough.
> 
> The GChecksum API is inadequate for QEMU's needs, due to its limited
> range of algorithms. We absolutely do not want different areas of
> the code using different APIs either. The goal of the crypto APIs is
> to provide a standard internal API for all cryptographic related
> operations for use across the whole codebase. This has clarified much
> of our code by removing countless #ifdef conditionals from the code
> and similar from the build system. It also facilitates people auditing
> QEMU use & implementation of crypto as there is only one place to look
> at to review. It also ensures that QEMU is only using certified secure
> crypto libraries, not some custom re-implementation of the crypto
> algorithms that have never been through a security review. Finally is
> ensures that QEMU correctly responds to runtime configurable changes,
> such as FIPS mode which restricts use of certain crypto algorithms
> at runtime, even if they're technically available at compile time.

Acutally, having said that, what we could do is to replace the no-op
stub hash impl, with a GCheckusum based impl. That way if neither
gcrypt or nettle are available, we can fallback to GChecksum for a
sub-set of the hash algorithms.

That would allow us to move the gcrypto_hash_supports() check out
of the quorum register method, and into its open() method, and
avoid any Makefile.objs conditionals.


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