| Message ID | 20170906124900.17354-1-famz@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v4] buildsys: Move crypto cflags/libs to per object variables | expand |
On Wed, 09/06 20:49, Fam Zheng wrote: > This patch groups the crypto objects into a few .mo objects based on > functional submodules, and moves inclusion conditions to *-objs > variables, then moves the global cflags/libs to the *-cflags and *-libs > variables. > > For init.o and cipher.o, which may or may not need the library flags > depending on config, adding flags and libs unconditionally doesn't hurt, > because if the library is not available, the variables are empty. This > makes less code. > > Signed-off-by: Fam Zheng <famz@redhat.com> Cc Dan. No idea why it was empty. Fam
On Fri, 09/08 14:31, Fam Zheng wrote: > On Wed, 09/06 20:49, Fam Zheng wrote: > > This patch groups the crypto objects into a few .mo objects based on > > functional submodules, and moves inclusion conditions to *-objs > > variables, then moves the global cflags/libs to the *-cflags and *-libs > > variables. > > > > For init.o and cipher.o, which may or may not need the library flags > > depending on config, adding flags and libs unconditionally doesn't hurt, > > because if the library is not available, the variables are empty. This > > makes less code. > > > > Signed-off-by: Fam Zheng <famz@redhat.com> > > Cc Dan. No idea why it was empty. Hmm, what is going on? Trying again with email address only.
On Wed, Sep 06, 2017 at 08:49:00PM +0800, Fam Zheng wrote: > This patch groups the crypto objects into a few .mo objects based on > functional submodules, and moves inclusion conditions to *-objs > variables, then moves the global cflags/libs to the *-cflags and *-libs > variables. > > For init.o and cipher.o, which may or may not need the library flags > depending on config, adding flags and libs unconditionally doesn't hurt, > because if the library is not available, the variables are empty. This > makes less code. > > Signed-off-by: Fam Zheng <famz@redhat.com> > > --- > > v4: Merge into one patch which is supposedly easier to manage and > review, and use .mo appraoch to avoid $(foreach) and $(eval) magics. I don't think using .mo is suitable here. You've used it as a generic mechanism for grouping .o files, but that is not what it does. There are special semantics around .mo rules that affect how the final binaries are linked. eg looking back at the description of .mo files [quote] commit c261d774fb9093d00e0938a19f502fb220f62718 Author: Fam Zheng <famz@redhat.com> Date: Mon Sep 1 18:35:10 2014 +0800 [...snip...] 3) When linking an executable, those .mo files in its "-y" variables are filtered out, and replaced by one or more -Wl,-u,$symbol flags. This is done in the added macro "process-archive-undefs". These "-Wl,-u,$symbol" flags will force ld to pull in the function definition from the archives when linking. Note that the .mo objects, that are actually meant to be linked in the executables, are already expanded in unnest-vars, before the linking command. So we are safe to simply filter out .mo for the purpose of pulling undefined symbols. process-archive-undefs works as this: For each ".mo", find all the undefined symbols in it, filter ones that are defined in the archives. For each of these symbols, generate a "-Wl,-u,$symbol" in the link command, and put them before archive names in the command line. [/quote] Based on this, I don't think I can ack this patch, because it can have unexpected consequences. > --- > configure | 15 +++++------- > crypto/Makefile.objs | 66 ++++++++++++++++++++++++++++++++++++-------------- > tests/Makefile.include | 10 ++++---- > 3 files changed, 59 insertions(+), 32 deletions(-) > > diff --git a/configure b/configure > index fb7e34a901..b37cd54bda 100755 > --- a/configure > +++ b/configure > @@ -2472,9 +2472,6 @@ if test "$gnutls" != "no"; then > if gnutls_works; then > gnutls_cflags=$($pkg_config --cflags gnutls) > gnutls_libs=$($pkg_config --libs gnutls) > - libs_softmmu="$gnutls_libs $libs_softmmu" > - libs_tools="$gnutls_libs $libs_tools" > - QEMU_CFLAGS="$QEMU_CFLAGS $gnutls_cflags" > gnutls="yes" > > # gnutls_rnd requires >= 2.11.0 > @@ -2568,9 +2565,6 @@ if test "$gcrypt" != "no"; then > then > gcrypt_libs="$gcrypt_libs -lgpg-error" > fi > - libs_softmmu="$gcrypt_libs $libs_softmmu" > - libs_tools="$gcrypt_libs $libs_tools" > - QEMU_CFLAGS="$QEMU_CFLAGS $gcrypt_cflags" > gcrypt="yes" > if test -z "$nettle"; then > nettle="no" > @@ -2616,9 +2610,6 @@ if test "$nettle" != "no"; then > nettle_cflags=$($pkg_config --cflags nettle) > nettle_libs=$($pkg_config --libs nettle) > nettle_version=$($pkg_config --modversion nettle) > - libs_softmmu="$nettle_libs $libs_softmmu" > - libs_tools="$nettle_libs $libs_tools" > - QEMU_CFLAGS="$QEMU_CFLAGS $nettle_cflags" > nettle="yes" > > cat > $TMPC << EOF > @@ -5713,12 +5704,16 @@ fi > echo "CONFIG_TLS_PRIORITY=\"$tls_priority\"" >> $config_host_mak > if test "$gnutls" = "yes" ; then > echo "CONFIG_GNUTLS=y" >> $config_host_mak > + echo "GNUTLS_CFLAGS=$gnutls_cflags" >> $config_host_mak > + echo "GNUTLS_LIBS=$gnutls_libs" >> $config_host_mak > fi > if test "$gnutls_rnd" = "yes" ; then > echo "CONFIG_GNUTLS_RND=y" >> $config_host_mak > fi > if test "$gcrypt" = "yes" ; then > echo "CONFIG_GCRYPT=y" >> $config_host_mak > + echo "GCRYPT_CFLAGS=$gcrypt_cflags" >> $config_host_mak > + echo "GCRYPT_LIBS=$gcrypt_libs" >> $config_host_mak > if test "$gcrypt_hmac" = "yes" ; then > echo "CONFIG_GCRYPT_HMAC=y" >> $config_host_mak > fi > @@ -5732,6 +5727,8 @@ if test "$nettle" = "yes" ; then > if test "$nettle_kdf" = "yes" ; then > echo "CONFIG_NETTLE_KDF=y" >> $config_host_mak > fi > + echo "NETTLE_CFLAGS=$nettle_cflags" >> $config_host_mak > + echo "NETTLE_LIBS=$nettle_libs" >> $config_host_mak > fi > if test "$tasn1" = "yes" ; then > echo "CONFIG_TASN1=y" >> $config_host_mak > diff --git a/crypto/Makefile.objs b/crypto/Makefile.objs > index 2b99e08062..a3ff1417c7 100644 > --- a/crypto/Makefile.objs > +++ b/crypto/Makefile.objs > @@ -1,29 +1,53 @@ > crypto-obj-y = init.o > -crypto-obj-y += hash.o > -crypto-obj-$(CONFIG_NETTLE) += hash-nettle.o > -crypto-obj-$(if $(CONFIG_NETTLE),n,$(CONFIG_GCRYPT)) += hash-gcrypt.o > -crypto-obj-$(if $(CONFIG_NETTLE),n,$(if $(CONFIG_GCRYPT),n,y)) += hash-glib.o > -crypto-obj-y += hmac.o > -crypto-obj-$(CONFIG_NETTLE) += hmac-nettle.o > -crypto-obj-$(CONFIG_GCRYPT_HMAC) += hmac-gcrypt.o > -crypto-obj-$(if $(CONFIG_NETTLE),n,$(if $(CONFIG_GCRYPT_HMAC),n,y)) += hmac-glib.o > +crypto-obj-y += hash.mo > + > +hash.mo-objs := hash.o \ > + $(if $(CONFIG_NETTLE), \ > + hash-nettle.o, \ > + $(if $(CONFIG_GCRYPT), hash-gcrypt.o, hash-glib.o)) > +hash.mo-cflags := $(NETTLE_CFLAGS) $(GCRYPT_CFLAGS) > +hash.mo-libs := $(NETTLE_LIBS) $(GCRYPT_LIBS) > + > +crypto-obj-y += hmac.mo > +hmac.mo-objs := hmac.o \ > + $(if $(CONFIG_NETTLE), \ > + hmac-nettle.o, \ > + $(if $(CONFIG_GCRYPT_HMAC), hmac-gcrypt.o, hmac-glib.o)) > +hmac.mo-cflags := $(NETTLE_CFLAGS) $(GCRYPT_CFLAGS) > +hmac.mo-libs := $(NETTLE_LIBS) $(GCRYPT_LIBS) > + > crypto-obj-y += aes.o > crypto-obj-y += desrfb.o > crypto-obj-y += cipher.o > crypto-obj-$(CONFIG_AF_ALG) += afalg.o > crypto-obj-$(CONFIG_AF_ALG) += cipher-afalg.o > crypto-obj-$(CONFIG_AF_ALG) += hash-afalg.o > -crypto-obj-y += tlscreds.o > -crypto-obj-y += tlscredsanon.o > -crypto-obj-y += tlscredsx509.o > -crypto-obj-y += tlssession.o > + > +crypto-obj-y += tls.mo > +tls.mo-objs := \ > + tlscreds.o \ > + tlscredsanon.o \ > + tlscredsx509.o \ > + tlssession.o > +tls.mo-cflags := $(GNUTLS_CFLAGS) > +tls.mo-libs := $(GNUTLS_LIBS) > + > crypto-obj-y += secret.o > -crypto-obj-$(CONFIG_GCRYPT) += random-gcrypt.o > -crypto-obj-$(if $(CONFIG_GCRYPT),n,$(CONFIG_GNUTLS_RND)) += random-gnutls.o > -crypto-obj-$(if $(CONFIG_GCRYPT),n,$(if $(CONFIG_GNUTLS_RND),n,y)) += random-platform.o > -crypto-obj-y += pbkdf.o > -crypto-obj-$(CONFIG_NETTLE_KDF) += pbkdf-nettle.o > -crypto-obj-$(if $(CONFIG_NETTLE_KDF),n,$(CONFIG_GCRYPT_KDF)) += pbkdf-gcrypt.o > + > +crypto-obj-y += random.mo > +random.mo-objs := \ > + $(if $(CONFIG_GCRYPT), random-gcrypt.o, \ > + $(if $(CONFIG_GNUTLS_RND), random-gnutls.o, random-platform.o)) > +random.mo-cflags := $(GNUTLS_CFLAGS) $(GCRYPT_CFLAGS) > +random.mo-libs := $(GNUTLS_LIBS) $(GCRYPT_LIBS) > + > +crypto-obj-y += pbkdf.mo > +pbkdf.mo-objs := pbkdf.o \ > + $(if $(CONFIG_NETTLE_KDF), pbkdf-nettle.o, \ > + $(if $(CONFIG_GCRYPT_KDF), pbkdf-gcrypt.o)) > +pbkdf.mo-cflags := $(NETTLE_CFLAGS) $(GCRYPT_CFLAGS) > +pbkdf.mo-libs := $(NETTLE_LIBS) $(GCRYPT_LIBS) > + > crypto-obj-y += ivgen.o > crypto-obj-y += ivgen-essiv.o > crypto-obj-y += ivgen-plain.o > @@ -34,6 +58,12 @@ crypto-obj-y += block.o > crypto-obj-y += block-qcow.o > crypto-obj-y += block-luks.o > > +init.o-cflags := $(GNUTLS_CFLAGS) $(GCRYPT_CFLAGS) > +init.o-libs := $(GNUTLS_LIBS) $(GCRYPT_LIBS) > + > +cipher.o-cflags := $(NETTLE_CFLAGS) $(GCRYPT_CFLAGS) > +cipher.o-libs := $(NETTLE_LIBS) $(GCRYPT_LIBS) > + > # Let the userspace emulators avoid linking gnutls/etc > crypto-aes-obj-y = aes.o > > diff --git a/tests/Makefile.include b/tests/Makefile.include > index fae5715e9c..d46c22d1ec 100644 > --- a/tests/Makefile.include > +++ b/tests/Makefile.include > @@ -677,15 +677,15 @@ tests/benchmark-crypto-cipher$(EXESUF): tests/benchmark-crypto-cipher.o $(test-c > tests/test-crypto-secret$(EXESUF): tests/test-crypto-secret.o $(test-crypto-obj-y) > tests/test-crypto-xts$(EXESUF): tests/test-crypto-xts.o $(test-crypto-obj-y) > > -tests/crypto-tls-x509-helpers.o-cflags := $(TASN1_CFLAGS) > -tests/crypto-tls-x509-helpers.o-libs := $(TASN1_LIBS) > -tests/pkix_asn1_tab.o-cflags := $(TASN1_CFLAGS) > +tests/crypto-tls-x509-helpers.o-cflags := $(TASN1_CFLAGS) $(GNUTLS_CFLAGS) > +tests/crypto-tls-x509-helpers.o-libs := $(TASN1_LIBS) $(GNUTLS_LIBS) > +tests/pkix_asn1_tab.o-cflags := $(TASN1_CFLAGS) $(GNUTLS_LIBS) > > -tests/test-crypto-tlscredsx509.o-cflags := $(TASN1_CFLAGS) > +tests/test-crypto-tlscredsx509.o-cflags := $(TASN1_CFLAGS) $(GNUTLS_CFLAGS) > tests/test-crypto-tlscredsx509$(EXESUF): tests/test-crypto-tlscredsx509.o \ > tests/crypto-tls-x509-helpers.o tests/pkix_asn1_tab.o $(test-crypto-obj-y) > > -tests/test-crypto-tlssession.o-cflags := $(TASN1_CFLAGS) > +tests/test-crypto-tlssession.o-cflags := $(TASN1_CFLAGS) $(GNUTLS_CFLAGS) > tests/test-crypto-tlssession$(EXESUF): tests/test-crypto-tlssession.o \ > tests/crypto-tls-x509-helpers.o tests/pkix_asn1_tab.o $(test-crypto-obj-y) > tests/test-io-task$(EXESUF): tests/test-io-task.o $(test-io-obj-y) > -- > 2.13.5 > Regards, Daniel
On Fri, 09/08 11:05, Daniel P. Berrange wrote: > On Wed, Sep 06, 2017 at 08:49:00PM +0800, Fam Zheng wrote: > > This patch groups the crypto objects into a few .mo objects based on > > functional submodules, and moves inclusion conditions to *-objs > > variables, then moves the global cflags/libs to the *-cflags and *-libs > > variables. > > > > For init.o and cipher.o, which may or may not need the library flags > > depending on config, adding flags and libs unconditionally doesn't hurt, > > because if the library is not available, the variables are empty. This > > makes less code. > > > > Signed-off-by: Fam Zheng <famz@redhat.com> > > > > --- > > > > v4: Merge into one patch which is supposedly easier to manage and > > review, and use .mo appraoch to avoid $(foreach) and $(eval) magics. > > I don't think using .mo is suitable here. You've used it as a generic > mechanism for grouping .o files, but that is not what it does. There > are special semantics around .mo rules that affect how the final > binaries are linked. Using .mo is okay here, but after a hindsight I think grouping by library (nettle.mo, gcrypt.mo, etc.) is better than grouping by functionality, for modularization in the future. But that also means assigning the cflags/libs variable cannot be simplified like this. > > eg looking back at the description of .mo files > > [quote] > commit c261d774fb9093d00e0938a19f502fb220f62718 > Author: Fam Zheng <famz@redhat.com> > Date: Mon Sep 1 18:35:10 2014 +0800 > > [...snip...] > > 3) When linking an executable, those .mo files in its "-y" variables are > filtered out, and replaced by one or more -Wl,-u,$symbol flags. This > is done in the added macro "process-archive-undefs". > > These "-Wl,-u,$symbol" flags will force ld to pull in the function > definition from the archives when linking. > > Note that the .mo objects, that are actually meant to be linked in > the executables, are already expanded in unnest-vars, before the > linking command. So we are safe to simply filter out .mo for the > purpose of pulling undefined symbols. > > process-archive-undefs works as this: For each ".mo", find all the > undefined symbols in it, filter ones that are defined in the > archives. For each of these symbols, generate a "-Wl,-u,$symbol" in > the link command, and put them before archive names in the command > line. > [/quote] > > Based on this, I don't think I can ack this patch, because it can > have unexpected consequences. This described the process-archive-undefs semantics of .mo, but not the essence of it. Basically .mo is just partial linking with the additional services of -cflags, -libs and the above -Wl,-u thing. I cannot think of any unexpected consequences with this change. We've had sdl.mo in ui/Makefile.objs for long, just for the same purpose of this patch, with no problem. Fam
On Fri, Sep 08, 2017 at 06:27:01PM +0800, Fam Zheng wrote: > On Fri, 09/08 11:05, Daniel P. Berrange wrote: > > On Wed, Sep 06, 2017 at 08:49:00PM +0800, Fam Zheng wrote: > > > This patch groups the crypto objects into a few .mo objects based on > > > functional submodules, and moves inclusion conditions to *-objs > > > variables, then moves the global cflags/libs to the *-cflags and *-libs > > > variables. > > > > > > For init.o and cipher.o, which may or may not need the library flags > > > depending on config, adding flags and libs unconditionally doesn't hurt, > > > because if the library is not available, the variables are empty. This > > > makes less code. > > > > > > Signed-off-by: Fam Zheng <famz@redhat.com> > > > > > > --- > > > > > > v4: Merge into one patch which is supposedly easier to manage and > > > review, and use .mo appraoch to avoid $(foreach) and $(eval) magics. > > > > I don't think using .mo is suitable here. You've used it as a generic > > mechanism for grouping .o files, but that is not what it does. There > > are special semantics around .mo rules that affect how the final > > binaries are linked. > > Using .mo is okay here, but after a hindsight I think grouping by library > (nettle.mo, gcrypt.mo, etc.) is better than grouping by functionality, for > modularization in the future. But that also means assigning the cflags/libs > variable cannot be simplified like this. > > > > > eg looking back at the description of .mo files > > > > [quote] > > commit c261d774fb9093d00e0938a19f502fb220f62718 > > Author: Fam Zheng <famz@redhat.com> > > Date: Mon Sep 1 18:35:10 2014 +0800 > > > > [...snip...] > > > > 3) When linking an executable, those .mo files in its "-y" variables are > > filtered out, and replaced by one or more -Wl,-u,$symbol flags. This > > is done in the added macro "process-archive-undefs". > > > > These "-Wl,-u,$symbol" flags will force ld to pull in the function > > definition from the archives when linking. > > > > Note that the .mo objects, that are actually meant to be linked in > > the executables, are already expanded in unnest-vars, before the > > linking command. So we are safe to simply filter out .mo for the > > purpose of pulling undefined symbols. > > > > process-archive-undefs works as this: For each ".mo", find all the > > undefined symbols in it, filter ones that are defined in the > > archives. For each of these symbols, generate a "-Wl,-u,$symbol" in > > the link command, and put them before archive names in the command > > line. > > [/quote] > > > > Based on this, I don't think I can ack this patch, because it can > > have unexpected consequences. > > This described the process-archive-undefs semantics of .mo, but not the essence > of it. Basically .mo is just partial linking with the additional services of > -cflags, -libs and the above -Wl,-u thing. I cannot think of any unexpected > consequences with this change. We've had sdl.mo in ui/Makefile.objs for long, > just for the same purpose of this patch, with no problem. While I'm in favour of moving the linker/compiler flags out of the global vars, I'm not convinced this impl is a step forward. We already have a mechanism for grouping object files - the 'NNNN-obj-y' variables we use throughout our Makefiles. This patch is adding a second level of grouping purely to work around the fact that we can't set linker/compiler flags on the NNN-obj-y variables we use. I think this second level of grouping makes the makefiles more complex than they ought to be. IOW, I'd rather see the rules fixed so that we can set variables against the existing grouping we have. eg crypto-obj-y-cflags := ... crypto-obj-y-libs := ... so we avoid having to introduce second level groups every time we want to set these cflags/libs. Regards, Daniel
On Fri, 09/08 11:36, Daniel P. Berrange wrote: > On Fri, Sep 08, 2017 at 06:27:01PM +0800, Fam Zheng wrote: > > On Fri, 09/08 11:05, Daniel P. Berrange wrote: > > > On Wed, Sep 06, 2017 at 08:49:00PM +0800, Fam Zheng wrote: > > > > This patch groups the crypto objects into a few .mo objects based on > > > > functional submodules, and moves inclusion conditions to *-objs > > > > variables, then moves the global cflags/libs to the *-cflags and *-libs > > > > variables. > > > > > > > > For init.o and cipher.o, which may or may not need the library flags > > > > depending on config, adding flags and libs unconditionally doesn't hurt, > > > > because if the library is not available, the variables are empty. This > > > > makes less code. > > > > > > > > Signed-off-by: Fam Zheng <famz@redhat.com> > > > > > > > > --- > > > > > > > > v4: Merge into one patch which is supposedly easier to manage and > > > > review, and use .mo appraoch to avoid $(foreach) and $(eval) magics. > > > > > > I don't think using .mo is suitable here. You've used it as a generic > > > mechanism for grouping .o files, but that is not what it does. There > > > are special semantics around .mo rules that affect how the final > > > binaries are linked. > > > > Using .mo is okay here, but after a hindsight I think grouping by library > > (nettle.mo, gcrypt.mo, etc.) is better than grouping by functionality, for > > modularization in the future. But that also means assigning the cflags/libs > > variable cannot be simplified like this. > > > > > > > > eg looking back at the description of .mo files > > > > > > [quote] > > > commit c261d774fb9093d00e0938a19f502fb220f62718 > > > Author: Fam Zheng <famz@redhat.com> > > > Date: Mon Sep 1 18:35:10 2014 +0800 > > > > > > [...snip...] > > > > > > 3) When linking an executable, those .mo files in its "-y" variables are > > > filtered out, and replaced by one or more -Wl,-u,$symbol flags. This > > > is done in the added macro "process-archive-undefs". > > > > > > These "-Wl,-u,$symbol" flags will force ld to pull in the function > > > definition from the archives when linking. > > > > > > Note that the .mo objects, that are actually meant to be linked in > > > the executables, are already expanded in unnest-vars, before the > > > linking command. So we are safe to simply filter out .mo for the > > > purpose of pulling undefined symbols. > > > > > > process-archive-undefs works as this: For each ".mo", find all the > > > undefined symbols in it, filter ones that are defined in the > > > archives. For each of these symbols, generate a "-Wl,-u,$symbol" in > > > the link command, and put them before archive names in the command > > > line. > > > [/quote] > > > > > > Based on this, I don't think I can ack this patch, because it can > > > have unexpected consequences. > > > > This described the process-archive-undefs semantics of .mo, but not the essence > > of it. Basically .mo is just partial linking with the additional services of > > -cflags, -libs and the above -Wl,-u thing. I cannot think of any unexpected > > consequences with this change. We've had sdl.mo in ui/Makefile.objs for long, > > just for the same purpose of this patch, with no problem. > > While I'm in favour of moving the linker/compiler flags out of the global > vars, I'm not convinced this impl is a step forward. > > We already have a mechanism for grouping object files - the 'NNNN-obj-y' > variables we use throughout our Makefiles. > > This patch is adding a second level of grouping purely to work around the > fact that we can't set linker/compiler flags on the NNN-obj-y variables > we use. I think this second level of grouping makes the makefiles more > complex than they ought to be. Not quite, it is actually a required step to modularization, which I'm inclined to get my hands on next. That is also why .mo was introduced. > > IOW, I'd rather see the rules fixed so that we can set variables against > the existing grouping we have. eg > > crypto-obj-y-cflags := ... > crypto-obj-y-libs := ... > > so we avoid having to introduce second level groups every time we want > to set these cflags/libs. This is certainly true, but taking the modularization work into account, .mo based -cflags and -libs are more natural and consistent. IMO we already have the latter, so other mechanisms are not really necessary. Remember how complex the general unnest-vars code is? I believe adding support to crypto-obj-y-cflags is more complex than (ab)using .mo objects, even if just for flags/libs localization. If you don't like introducing {nettle,gcrypt,gnutls}.mo for now, we can probably defer it to the time when crypto subsystem is modularized. Either way let's drop this patch for now. Fam
On Fri, Sep 08, 2017 at 06:58:53PM +0800, Fam Zheng wrote: > On Fri, 09/08 11:36, Daniel P. Berrange wrote: > > On Fri, Sep 08, 2017 at 06:27:01PM +0800, Fam Zheng wrote: > > > On Fri, 09/08 11:05, Daniel P. Berrange wrote: > > > > On Wed, Sep 06, 2017 at 08:49:00PM +0800, Fam Zheng wrote: > > > > > This patch groups the crypto objects into a few .mo objects based on > > > > > functional submodules, and moves inclusion conditions to *-objs > > > > > variables, then moves the global cflags/libs to the *-cflags and *-libs > > > > > variables. > > > > > > > > > > For init.o and cipher.o, which may or may not need the library flags > > > > > depending on config, adding flags and libs unconditionally doesn't hurt, > > > > > because if the library is not available, the variables are empty. This > > > > > makes less code. > > > > > > > > > > Signed-off-by: Fam Zheng <famz@redhat.com> > > > > > > > > > > --- > > > > > > > > > > v4: Merge into one patch which is supposedly easier to manage and > > > > > review, and use .mo appraoch to avoid $(foreach) and $(eval) magics. > > > > > > > > I don't think using .mo is suitable here. You've used it as a generic > > > > mechanism for grouping .o files, but that is not what it does. There > > > > are special semantics around .mo rules that affect how the final > > > > binaries are linked. > > > > > > Using .mo is okay here, but after a hindsight I think grouping by library > > > (nettle.mo, gcrypt.mo, etc.) is better than grouping by functionality, for > > > modularization in the future. But that also means assigning the cflags/libs > > > variable cannot be simplified like this. > > > > > > > > > > > eg looking back at the description of .mo files > > > > > > > > [quote] > > > > commit c261d774fb9093d00e0938a19f502fb220f62718 > > > > Author: Fam Zheng <famz@redhat.com> > > > > Date: Mon Sep 1 18:35:10 2014 +0800 > > > > > > > > [...snip...] > > > > > > > > 3) When linking an executable, those .mo files in its "-y" variables are > > > > filtered out, and replaced by one or more -Wl,-u,$symbol flags. This > > > > is done in the added macro "process-archive-undefs". > > > > > > > > These "-Wl,-u,$symbol" flags will force ld to pull in the function > > > > definition from the archives when linking. > > > > > > > > Note that the .mo objects, that are actually meant to be linked in > > > > the executables, are already expanded in unnest-vars, before the > > > > linking command. So we are safe to simply filter out .mo for the > > > > purpose of pulling undefined symbols. > > > > > > > > process-archive-undefs works as this: For each ".mo", find all the > > > > undefined symbols in it, filter ones that are defined in the > > > > archives. For each of these symbols, generate a "-Wl,-u,$symbol" in > > > > the link command, and put them before archive names in the command > > > > line. > > > > [/quote] > > > > > > > > Based on this, I don't think I can ack this patch, because it can > > > > have unexpected consequences. > > > > > > This described the process-archive-undefs semantics of .mo, but not the essence > > > of it. Basically .mo is just partial linking with the additional services of > > > -cflags, -libs and the above -Wl,-u thing. I cannot think of any unexpected > > > consequences with this change. We've had sdl.mo in ui/Makefile.objs for long, > > > just for the same purpose of this patch, with no problem. > > > > While I'm in favour of moving the linker/compiler flags out of the global > > vars, I'm not convinced this impl is a step forward. > > > > We already have a mechanism for grouping object files - the 'NNNN-obj-y' > > variables we use throughout our Makefiles. > > > > This patch is adding a second level of grouping purely to work around the > > fact that we can't set linker/compiler flags on the NNN-obj-y variables > > we use. I think this second level of grouping makes the makefiles more > > complex than they ought to be. > > Not quite, it is actually a required step to modularization, which I'm inclined > to get my hands on next. That is also why .mo was introduced. > > > > > IOW, I'd rather see the rules fixed so that we can set variables against > > the existing grouping we have. eg > > > > crypto-obj-y-cflags := ... > > crypto-obj-y-libs := ... > > > > so we avoid having to introduce second level groups every time we want > > to set these cflags/libs. > > This is certainly true, but taking the modularization work into account, .mo > based -cflags and -libs are more natural and consistent. IMO we already have the > latter, so other mechanisms are not really necessary. Remember how complex the > general unnest-vars code is? I believe adding support to crypto-obj-y-cflags is > more complex than (ab)using .mo objects, even if just for flags/libs > localization. > > If you don't like introducing {nettle,gcrypt,gnutls}.mo for now, we can probably > defer it to the time when crypto subsystem is modularized. I don't anticipate the crypot subsystem ever being modularized - it is really core functionality used across all other subsystems (block layer, chardev, ui, migration, and more) Regards, Daniel
On Fri, 09/08 12:00, Daniel P. Berrange wrote: > On Fri, Sep 08, 2017 at 06:58:53PM +0800, Fam Zheng wrote: > > On Fri, 09/08 11:36, Daniel P. Berrange wrote: > > > On Fri, Sep 08, 2017 at 06:27:01PM +0800, Fam Zheng wrote: > > > > On Fri, 09/08 11:05, Daniel P. Berrange wrote: > > > > > On Wed, Sep 06, 2017 at 08:49:00PM +0800, Fam Zheng wrote: > > > > > > This patch groups the crypto objects into a few .mo objects based on > > > > > > functional submodules, and moves inclusion conditions to *-objs > > > > > > variables, then moves the global cflags/libs to the *-cflags and *-libs > > > > > > variables. > > > > > > > > > > > > For init.o and cipher.o, which may or may not need the library flags > > > > > > depending on config, adding flags and libs unconditionally doesn't hurt, > > > > > > because if the library is not available, the variables are empty. This > > > > > > makes less code. > > > > > > > > > > > > Signed-off-by: Fam Zheng <famz@redhat.com> > > > > > > > > > > > > --- > > > > > > > > > > > > v4: Merge into one patch which is supposedly easier to manage and > > > > > > review, and use .mo appraoch to avoid $(foreach) and $(eval) magics. > > > > > > > > > > I don't think using .mo is suitable here. You've used it as a generic > > > > > mechanism for grouping .o files, but that is not what it does. There > > > > > are special semantics around .mo rules that affect how the final > > > > > binaries are linked. > > > > > > > > Using .mo is okay here, but after a hindsight I think grouping by library > > > > (nettle.mo, gcrypt.mo, etc.) is better than grouping by functionality, for > > > > modularization in the future. But that also means assigning the cflags/libs > > > > variable cannot be simplified like this. > > > > > > > > > > > > > > eg looking back at the description of .mo files > > > > > > > > > > [quote] > > > > > commit c261d774fb9093d00e0938a19f502fb220f62718 > > > > > Author: Fam Zheng <famz@redhat.com> > > > > > Date: Mon Sep 1 18:35:10 2014 +0800 > > > > > > > > > > [...snip...] > > > > > > > > > > 3) When linking an executable, those .mo files in its "-y" variables are > > > > > filtered out, and replaced by one or more -Wl,-u,$symbol flags. This > > > > > is done in the added macro "process-archive-undefs". > > > > > > > > > > These "-Wl,-u,$symbol" flags will force ld to pull in the function > > > > > definition from the archives when linking. > > > > > > > > > > Note that the .mo objects, that are actually meant to be linked in > > > > > the executables, are already expanded in unnest-vars, before the > > > > > linking command. So we are safe to simply filter out .mo for the > > > > > purpose of pulling undefined symbols. > > > > > > > > > > process-archive-undefs works as this: For each ".mo", find all the > > > > > undefined symbols in it, filter ones that are defined in the > > > > > archives. For each of these symbols, generate a "-Wl,-u,$symbol" in > > > > > the link command, and put them before archive names in the command > > > > > line. > > > > > [/quote] > > > > > > > > > > Based on this, I don't think I can ack this patch, because it can > > > > > have unexpected consequences. > > > > > > > > This described the process-archive-undefs semantics of .mo, but not the essence > > > > of it. Basically .mo is just partial linking with the additional services of > > > > -cflags, -libs and the above -Wl,-u thing. I cannot think of any unexpected > > > > consequences with this change. We've had sdl.mo in ui/Makefile.objs for long, > > > > just for the same purpose of this patch, with no problem. > > > > > > While I'm in favour of moving the linker/compiler flags out of the global > > > vars, I'm not convinced this impl is a step forward. > > > > > > We already have a mechanism for grouping object files - the 'NNNN-obj-y' > > > variables we use throughout our Makefiles. > > > > > > This patch is adding a second level of grouping purely to work around the > > > fact that we can't set linker/compiler flags on the NNN-obj-y variables > > > we use. I think this second level of grouping makes the makefiles more > > > complex than they ought to be. > > > > Not quite, it is actually a required step to modularization, which I'm inclined > > to get my hands on next. That is also why .mo was introduced. > > > > > > > > IOW, I'd rather see the rules fixed so that we can set variables against > > > the existing grouping we have. eg > > > > > > crypto-obj-y-cflags := ... > > > crypto-obj-y-libs := ... > > > > > > so we avoid having to introduce second level groups every time we want > > > to set these cflags/libs. > > > > This is certainly true, but taking the modularization work into account, .mo > > based -cflags and -libs are more natural and consistent. IMO we already have the > > latter, so other mechanisms are not really necessary. Remember how complex the > > general unnest-vars code is? I believe adding support to crypto-obj-y-cflags is > > more complex than (ab)using .mo objects, even if just for flags/libs > > localization. > > > > If you don't like introducing {nettle,gcrypt,gnutls}.mo for now, we can probably > > defer it to the time when crypto subsystem is modularized. > > I don't anticipate the crypot subsystem ever being modularized - it is > really core functionality used across all other subsystems (block layer, > chardev, ui, migration, and more) I get your point that crypto is a fundamental thing, "optionally secure" is not what I meant. But moduarization still has the advantage of offering more flexibility to end users, potentially. Crypto backends could be shipped as qemu-crypto-{nettle,gcrypt,gnutls} packages, and depending on which are installed and which are not, the core crypto code in QEMU can pick the suitable implementation at runtime, based on a hardcoded priority or even an option. To be "secure by default", qemu-crypto-nettle could be a hard requirement of qemu core package. Is it worth the effort? Fam
On Fri, Sep 08, 2017 at 07:23:52PM +0800, Fam Zheng wrote: > On Fri, 09/08 12:00, Daniel P. Berrange wrote: > > On Fri, Sep 08, 2017 at 06:58:53PM +0800, Fam Zheng wrote: > > > If you don't like introducing {nettle,gcrypt,gnutls}.mo for now, we can probably > > > defer it to the time when crypto subsystem is modularized. > > > > I don't anticipate the crypot subsystem ever being modularized - it is > > really core functionality used across all other subsystems (block layer, > > chardev, ui, migration, and more) > > I get your point that crypto is a fundamental thing, "optionally secure" is not > what I meant. But moduarization still has the advantage of offering more > flexibility to end users, potentially. Crypto backends could be shipped as > qemu-crypto-{nettle,gcrypt,gnutls} packages, and depending on which are > installed and which are not, the core crypto code in QEMU can pick the suitable > implementation at runtime, based on a hardcoded priority or even an option. There's no choice between gnutls vs the other two - it is always a case of building gnutls *and* either nettle or gcrypt. We pick nettle or gcrypt based on which is used by gnutls, to minimise number of different crypto libraries we load. So dynamically picking them at runtime makese no sense. > To be "secure by default", qemu-crypto-nettle could be a hard requirement of > qemu core package. > > Is it worth the effort? I don't really think that is useful. You should think of the crypto stuff as being part of the 'libqemuutil.la' library - the only reason it is not linked into that is because we wanted to avoid adds deps on the userspace emulators - everything else we build wants it present. Regards, Daniel
diff --git a/configure b/configure index fb7e34a901..b37cd54bda 100755 --- a/configure +++ b/configure @@ -2472,9 +2472,6 @@ if test "$gnutls" != "no"; then if gnutls_works; then gnutls_cflags=$($pkg_config --cflags gnutls) gnutls_libs=$($pkg_config --libs gnutls) - libs_softmmu="$gnutls_libs $libs_softmmu" - libs_tools="$gnutls_libs $libs_tools" - QEMU_CFLAGS="$QEMU_CFLAGS $gnutls_cflags" gnutls="yes" # gnutls_rnd requires >= 2.11.0 @@ -2568,9 +2565,6 @@ if test "$gcrypt" != "no"; then then gcrypt_libs="$gcrypt_libs -lgpg-error" fi - libs_softmmu="$gcrypt_libs $libs_softmmu" - libs_tools="$gcrypt_libs $libs_tools" - QEMU_CFLAGS="$QEMU_CFLAGS $gcrypt_cflags" gcrypt="yes" if test -z "$nettle"; then nettle="no" @@ -2616,9 +2610,6 @@ if test "$nettle" != "no"; then nettle_cflags=$($pkg_config --cflags nettle) nettle_libs=$($pkg_config --libs nettle) nettle_version=$($pkg_config --modversion nettle) - libs_softmmu="$nettle_libs $libs_softmmu" - libs_tools="$nettle_libs $libs_tools" - QEMU_CFLAGS="$QEMU_CFLAGS $nettle_cflags" nettle="yes" cat > $TMPC << EOF @@ -5713,12 +5704,16 @@ fi echo "CONFIG_TLS_PRIORITY=\"$tls_priority\"" >> $config_host_mak if test "$gnutls" = "yes" ; then echo "CONFIG_GNUTLS=y" >> $config_host_mak + echo "GNUTLS_CFLAGS=$gnutls_cflags" >> $config_host_mak + echo "GNUTLS_LIBS=$gnutls_libs" >> $config_host_mak fi if test "$gnutls_rnd" = "yes" ; then echo "CONFIG_GNUTLS_RND=y" >> $config_host_mak fi if test "$gcrypt" = "yes" ; then echo "CONFIG_GCRYPT=y" >> $config_host_mak + echo "GCRYPT_CFLAGS=$gcrypt_cflags" >> $config_host_mak + echo "GCRYPT_LIBS=$gcrypt_libs" >> $config_host_mak if test "$gcrypt_hmac" = "yes" ; then echo "CONFIG_GCRYPT_HMAC=y" >> $config_host_mak fi @@ -5732,6 +5727,8 @@ if test "$nettle" = "yes" ; then if test "$nettle_kdf" = "yes" ; then echo "CONFIG_NETTLE_KDF=y" >> $config_host_mak fi + echo "NETTLE_CFLAGS=$nettle_cflags" >> $config_host_mak + echo "NETTLE_LIBS=$nettle_libs" >> $config_host_mak fi if test "$tasn1" = "yes" ; then echo "CONFIG_TASN1=y" >> $config_host_mak diff --git a/crypto/Makefile.objs b/crypto/Makefile.objs index 2b99e08062..a3ff1417c7 100644 --- a/crypto/Makefile.objs +++ b/crypto/Makefile.objs @@ -1,29 +1,53 @@ crypto-obj-y = init.o -crypto-obj-y += hash.o -crypto-obj-$(CONFIG_NETTLE) += hash-nettle.o -crypto-obj-$(if $(CONFIG_NETTLE),n,$(CONFIG_GCRYPT)) += hash-gcrypt.o -crypto-obj-$(if $(CONFIG_NETTLE),n,$(if $(CONFIG_GCRYPT),n,y)) += hash-glib.o -crypto-obj-y += hmac.o -crypto-obj-$(CONFIG_NETTLE) += hmac-nettle.o -crypto-obj-$(CONFIG_GCRYPT_HMAC) += hmac-gcrypt.o -crypto-obj-$(if $(CONFIG_NETTLE),n,$(if $(CONFIG_GCRYPT_HMAC),n,y)) += hmac-glib.o +crypto-obj-y += hash.mo + +hash.mo-objs := hash.o \ + $(if $(CONFIG_NETTLE), \ + hash-nettle.o, \ + $(if $(CONFIG_GCRYPT), hash-gcrypt.o, hash-glib.o)) +hash.mo-cflags := $(NETTLE_CFLAGS) $(GCRYPT_CFLAGS) +hash.mo-libs := $(NETTLE_LIBS) $(GCRYPT_LIBS) + +crypto-obj-y += hmac.mo +hmac.mo-objs := hmac.o \ + $(if $(CONFIG_NETTLE), \ + hmac-nettle.o, \ + $(if $(CONFIG_GCRYPT_HMAC), hmac-gcrypt.o, hmac-glib.o)) +hmac.mo-cflags := $(NETTLE_CFLAGS) $(GCRYPT_CFLAGS) +hmac.mo-libs := $(NETTLE_LIBS) $(GCRYPT_LIBS) + crypto-obj-y += aes.o crypto-obj-y += desrfb.o crypto-obj-y += cipher.o crypto-obj-$(CONFIG_AF_ALG) += afalg.o crypto-obj-$(CONFIG_AF_ALG) += cipher-afalg.o crypto-obj-$(CONFIG_AF_ALG) += hash-afalg.o -crypto-obj-y += tlscreds.o -crypto-obj-y += tlscredsanon.o -crypto-obj-y += tlscredsx509.o -crypto-obj-y += tlssession.o + +crypto-obj-y += tls.mo +tls.mo-objs := \ + tlscreds.o \ + tlscredsanon.o \ + tlscredsx509.o \ + tlssession.o +tls.mo-cflags := $(GNUTLS_CFLAGS) +tls.mo-libs := $(GNUTLS_LIBS) + crypto-obj-y += secret.o -crypto-obj-$(CONFIG_GCRYPT) += random-gcrypt.o -crypto-obj-$(if $(CONFIG_GCRYPT),n,$(CONFIG_GNUTLS_RND)) += random-gnutls.o -crypto-obj-$(if $(CONFIG_GCRYPT),n,$(if $(CONFIG_GNUTLS_RND),n,y)) += random-platform.o -crypto-obj-y += pbkdf.o -crypto-obj-$(CONFIG_NETTLE_KDF) += pbkdf-nettle.o -crypto-obj-$(if $(CONFIG_NETTLE_KDF),n,$(CONFIG_GCRYPT_KDF)) += pbkdf-gcrypt.o + +crypto-obj-y += random.mo +random.mo-objs := \ + $(if $(CONFIG_GCRYPT), random-gcrypt.o, \ + $(if $(CONFIG_GNUTLS_RND), random-gnutls.o, random-platform.o)) +random.mo-cflags := $(GNUTLS_CFLAGS) $(GCRYPT_CFLAGS) +random.mo-libs := $(GNUTLS_LIBS) $(GCRYPT_LIBS) + +crypto-obj-y += pbkdf.mo +pbkdf.mo-objs := pbkdf.o \ + $(if $(CONFIG_NETTLE_KDF), pbkdf-nettle.o, \ + $(if $(CONFIG_GCRYPT_KDF), pbkdf-gcrypt.o)) +pbkdf.mo-cflags := $(NETTLE_CFLAGS) $(GCRYPT_CFLAGS) +pbkdf.mo-libs := $(NETTLE_LIBS) $(GCRYPT_LIBS) + crypto-obj-y += ivgen.o crypto-obj-y += ivgen-essiv.o crypto-obj-y += ivgen-plain.o @@ -34,6 +58,12 @@ crypto-obj-y += block.o crypto-obj-y += block-qcow.o crypto-obj-y += block-luks.o +init.o-cflags := $(GNUTLS_CFLAGS) $(GCRYPT_CFLAGS) +init.o-libs := $(GNUTLS_LIBS) $(GCRYPT_LIBS) + +cipher.o-cflags := $(NETTLE_CFLAGS) $(GCRYPT_CFLAGS) +cipher.o-libs := $(NETTLE_LIBS) $(GCRYPT_LIBS) + # Let the userspace emulators avoid linking gnutls/etc crypto-aes-obj-y = aes.o diff --git a/tests/Makefile.include b/tests/Makefile.include index fae5715e9c..d46c22d1ec 100644 --- a/tests/Makefile.include +++ b/tests/Makefile.include @@ -677,15 +677,15 @@ tests/benchmark-crypto-cipher$(EXESUF): tests/benchmark-crypto-cipher.o $(test-c tests/test-crypto-secret$(EXESUF): tests/test-crypto-secret.o $(test-crypto-obj-y) tests/test-crypto-xts$(EXESUF): tests/test-crypto-xts.o $(test-crypto-obj-y) -tests/crypto-tls-x509-helpers.o-cflags := $(TASN1_CFLAGS) -tests/crypto-tls-x509-helpers.o-libs := $(TASN1_LIBS) -tests/pkix_asn1_tab.o-cflags := $(TASN1_CFLAGS) +tests/crypto-tls-x509-helpers.o-cflags := $(TASN1_CFLAGS) $(GNUTLS_CFLAGS) +tests/crypto-tls-x509-helpers.o-libs := $(TASN1_LIBS) $(GNUTLS_LIBS) +tests/pkix_asn1_tab.o-cflags := $(TASN1_CFLAGS) $(GNUTLS_LIBS) -tests/test-crypto-tlscredsx509.o-cflags := $(TASN1_CFLAGS) +tests/test-crypto-tlscredsx509.o-cflags := $(TASN1_CFLAGS) $(GNUTLS_CFLAGS) tests/test-crypto-tlscredsx509$(EXESUF): tests/test-crypto-tlscredsx509.o \ tests/crypto-tls-x509-helpers.o tests/pkix_asn1_tab.o $(test-crypto-obj-y) -tests/test-crypto-tlssession.o-cflags := $(TASN1_CFLAGS) +tests/test-crypto-tlssession.o-cflags := $(TASN1_CFLAGS) $(GNUTLS_CFLAGS) tests/test-crypto-tlssession$(EXESUF): tests/test-crypto-tlssession.o \ tests/crypto-tls-x509-helpers.o tests/pkix_asn1_tab.o $(test-crypto-obj-y) tests/test-io-task$(EXESUF): tests/test-io-task.o $(test-io-obj-y)
This patch groups the crypto objects into a few .mo objects based on functional submodules, and moves inclusion conditions to *-objs variables, then moves the global cflags/libs to the *-cflags and *-libs variables. For init.o and cipher.o, which may or may not need the library flags depending on config, adding flags and libs unconditionally doesn't hurt, because if the library is not available, the variables are empty. This makes less code. Signed-off-by: Fam Zheng <famz@redhat.com> --- v4: Merge into one patch which is supposedly easier to manage and review, and use .mo appraoch to avoid $(foreach) and $(eval) magics. Fixes the gcrypt patch typo in v3 while doing this. --- configure | 15 +++++------- crypto/Makefile.objs | 66 ++++++++++++++++++++++++++++++++++++-------------- tests/Makefile.include | 10 ++++---- 3 files changed, 59 insertions(+), 32 deletions(-)