1. Patchew
  2. Libvirt
  3. [libvirt] [PATCH 0/3] Couple of improvements
  4. View patch

[libvirt] [PATCH 3/3] tools: Enable warnings for more binaries/libs

Michal Privoznik posted 3 patches 7 years, 5 months ago
[libvirt] [PATCH 3/3] tools: Enable warnings for more binaries/libs
Posted by Michal Privoznik 7 years, 5 months ago 
Because WARN_CFLAGS and COVERAGE_CFLAGS are not set globally, we
rely on each binary built to include WARN_CFLAGS/COVERAGE_CFLAGS.
But it is easy to forget those - e.g. libvirt_shell.la. However,
don't enable WARN_FLAGS (i.e. don't include AM_CFLAGS) for
wireshark plugin - parts of that code are generated and trigger
some warnings.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
 tools/Makefile.am | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/tools/Makefile.am b/tools/Makefile.am
index a844dcbbc..9bbb1a838 100644
--- a/tools/Makefile.am
+++ b/tools/Makefile.am
@@ -25,6 +25,11 @@ INCLUDES = \
 
 WARN_CFLAGS += $(STRICT_FRAME_LIMIT_CFLAGS)
 
+AM_CFLAGS = \
+	$(WARN_CFLAGS) \
+	$(COVERAGE_CFLAGS) \
+	$(NULL)
+
 AM_LDFLAGS = \
 	$(RELRO_LDFLAGS) \
 	$(NO_INDIRECT_LDFLAGS) \
@@ -182,8 +187,8 @@ virt_host_validate_LDADD = \
 		$(NULL)
 
 virt_host_validate_CFLAGS = \
+		$(AM_CFLAGS) \
 		$(LIBXML_CFLAGS) \
-		$(WARN_CFLAGS) \
 		$(PIE_CFLAGS) \
 		$(COVERAGE_CFLAGS) \
 		$(NULL)
@@ -208,8 +213,8 @@ virt_login_shell_LDADD = \
 
 virt_login_shell_CFLAGS = \
 		-DLIBVIRT_SETUID_RPC_CLIENT \
+		$(AM_CFLAGS) \
 		$(LIBXML_CFLAGS) \
-		$(WARN_CFLAGS) \
 		$(PIE_CFLAGS) \
 		$(COVERAGE_CFLAGS)
 
@@ -241,7 +246,7 @@ virsh_LDADD = \
 		../src/libvirt-qemu.la \
 		libvirt_shell.la
 virsh_CFLAGS = \
-		$(WARN_CFLAGS) \
+		$(AM_CFLAGS) \
 		$(PIE_CFLAGS) \
 		$(COVERAGE_CFLAGS) \
 		$(LIBXML_CFLAGS) \
@@ -263,7 +268,7 @@ virt_admin_LDADD = \
 		$(LIBXML_LIBS) \
 		$(NULL)
 virt_admin_CFLAGS = \
-		$(WARN_CFLAGS) \
+		$(AM_CFLAGS) \
 		$(PIE_CFLAGS) \
 		$(COVERAGE_CFLAGS) \
 		$(LIBXML_CFLAGS) \
@@ -502,9 +507,7 @@ nss_libnss_libvirt_impl_la_SOURCES = \
 nss_libnss_libvirt_impl_la_CFLAGS = \
 	-DLIBVIRT_NSS \
 	$(AM_CFLAGS) \
-	$(WARN_CFLAGS) \
 	$(PIE_CFLAGS) \
-	$(COVERAGE_CFLAGS) \
 	$(LIBXML_CFLAGS)
 
 nss_libnss_libvirt_impl_la_LIBADD = \
@@ -532,9 +535,7 @@ nss_libnss_libvirt_guest_impl_la_CFLAGS = \
 	-DLIBVIRT_NSS \
 	-DLIBVIRT_NSS_GUEST \
 	$(AM_CFLAGS) \
-	$(WARN_CFLAGS) \
 	$(PIE_CFLAGS) \
-	$(COVERAGE_CFLAGS) \
 	$(LIBXML_CFLAGS)
 
 nss_libnss_libvirt_guest_impl_la_LIBADD = \
-- 
2.13.6

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 3/3] tools: Enable warnings for more binaries/libs
Posted by Erik Skultety 7 years, 5 months ago 
On Thu, Nov 16, 2017 at 02:49:29PM +0100, Michal Privoznik wrote:
> Because WARN_CFLAGS and COVERAGE_CFLAGS are not set globally, we
> rely on each binary built to include WARN_CFLAGS/COVERAGE_CFLAGS.
> But it is easy to forget those - e.g. libvirt_shell.la. However,
> don't enable WARN_FLAGS (i.e. don't include AM_CFLAGS) for
> wireshark plugin - parts of that code are generated and trigger
> some warnings.
>
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> ---
>  tools/Makefile.am | 17 +++++++++--------
>  1 file changed, 9 insertions(+), 8 deletions(-)
>
> diff --git a/tools/Makefile.am b/tools/Makefile.am
> index a844dcbbc..9bbb1a838 100644
> --- a/tools/Makefile.am
> +++ b/tools/Makefile.am
> @@ -25,6 +25,11 @@ INCLUDES = \
>
>  WARN_CFLAGS += $(STRICT_FRAME_LIMIT_CFLAGS)
>
> +AM_CFLAGS = \
> +	$(WARN_CFLAGS) \
> +	$(COVERAGE_CFLAGS) \
> +	$(NULL)

It appears to me that every binary's CFLAGS variable also includes PIE_CFLAGS,
any reason for not moving them along the other ones?

> +
>  AM_LDFLAGS = \
>  	$(RELRO_LDFLAGS) \
>  	$(NO_INDIRECT_LDFLAGS) \
> @@ -182,8 +187,8 @@ virt_host_validate_LDADD = \
>  		$(NULL)
>
>  virt_host_validate_CFLAGS = \
> +		$(AM_CFLAGS) \
>  		$(LIBXML_CFLAGS) \
> -		$(WARN_CFLAGS) \
>  		$(PIE_CFLAGS) \
>  		$(COVERAGE_CFLAGS) \

I believe ^this one can be dropped as well now.

>  		$(NULL)
> @@ -208,8 +213,8 @@ virt_login_shell_LDADD = \
>
>  virt_login_shell_CFLAGS = \
>  		-DLIBVIRT_SETUID_RPC_CLIENT \
> +		$(AM_CFLAGS) \
>  		$(LIBXML_CFLAGS) \
> -		$(WARN_CFLAGS) \
>  		$(PIE_CFLAGS) \
>  		$(COVERAGE_CFLAGS)

...^here too...

>
> @@ -241,7 +246,7 @@ virsh_LDADD = \
>  		../src/libvirt-qemu.la \
>  		libvirt_shell.la
>  virsh_CFLAGS = \
> -		$(WARN_CFLAGS) \
> +		$(AM_CFLAGS) \
>  		$(PIE_CFLAGS) \
>  		$(COVERAGE_CFLAGS) \

...^here as well...

>  		$(LIBXML_CFLAGS) \
> @@ -263,7 +268,7 @@ virt_admin_LDADD = \
>  		$(LIBXML_LIBS) \
>  		$(NULL)
>  virt_admin_CFLAGS = \
> -		$(WARN_CFLAGS) \
> +		$(AM_CFLAGS) \
>  		$(PIE_CFLAGS) \
>  		$(COVERAGE_CFLAGS) \

...aand ^here too...

Reviewed-by: Erik Skultety <eskultet@redhat.com>

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 3/3] tools: Enable warnings for more binaries/libs
Posted by Peter Krempa 7 years, 5 months ago 
On Mon, Nov 20, 2017 at 17:36:54 +0100, Erik Skultety wrote:
> On Thu, Nov 16, 2017 at 02:49:29PM +0100, Michal Privoznik wrote:
> > Because WARN_CFLAGS and COVERAGE_CFLAGS are not set globally, we
> > rely on each binary built to include WARN_CFLAGS/COVERAGE_CFLAGS.
> > But it is easy to forget those - e.g. libvirt_shell.la. However,
> > don't enable WARN_FLAGS (i.e. don't include AM_CFLAGS) for
> > wireshark plugin - parts of that code are generated and trigger
> > some warnings.
> >
> > Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> > ---
> >  tools/Makefile.am | 17 +++++++++--------
> >  1 file changed, 9 insertions(+), 8 deletions(-)

This commit broke build on OSX [1] since apparently readline has
different definition for some internal variables:

vsh.c:2903:22: error: assigning to 'char *' from 'const char *' discards qualifiers [-Werror,-Wincompatible-pointer-types-discards-qualifiers]

    rl_readline_name = ctl->name;

                     ^ ~~~~~~~~~

vsh.c:2908:36: error: assigning to 'char *' from 'const char [16]' discards qualifiers [-Werror,-Wincompatible-pointer-types-discards-qualifiers]

    rl_basic_word_break_characters = " \t\n\\`@$><=;|&{(";

                                   ^ ~~~~~~~~~~~~~~~~~~~~

2 errors generated.

https://travis-ci.org/libvirt/libvirt/jobs/305251968
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 3/3] tools: Enable warnings for more binaries/libs
Posted by Michal Privoznik 7 years, 5 months ago 
On 11/23/2017 03:10 PM, Peter Krempa wrote:
> On Mon, Nov 20, 2017 at 17:36:54 +0100, Erik Skultety wrote:
>> On Thu, Nov 16, 2017 at 02:49:29PM +0100, Michal Privoznik wrote:
>>> Because WARN_CFLAGS and COVERAGE_CFLAGS are not set globally, we
>>> rely on each binary built to include WARN_CFLAGS/COVERAGE_CFLAGS.
>>> But it is easy to forget those - e.g. libvirt_shell.la. However,
>>> don't enable WARN_FLAGS (i.e. don't include AM_CFLAGS) for
>>> wireshark plugin - parts of that code are generated and trigger
>>> some warnings.
>>>
>>> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>>> ---
>>>  tools/Makefile.am | 17 +++++++++--------
>>>  1 file changed, 9 insertions(+), 8 deletions(-)
> 
> This commit broke build on OSX [1] since apparently readline has
> different definition for some internal variables:
> 
> vsh.c:2903:22: error: assigning to 'char *' from 'const char *' discards qualifiers [-Werror,-Wincompatible-pointer-types-discards-qualifiers]
> 
>     rl_readline_name = ctl->name;
> 
>                      ^ ~~~~~~~~~
> 
> vsh.c:2908:36: error: assigning to 'char *' from 'const char [16]' discards qualifiers [-Werror,-Wincompatible-pointer-types-discards-qualifiers]
> 
>     rl_basic_word_break_characters = " \t\n\\`@$><=;|&{(";
> 
>                                    ^ ~~~~~~~~~~~~~~~~~~~~
> 
> 2 errors generated.
> 
> https://travis-ci.org/libvirt/libvirt/jobs/305251968
> 

Right. I've seen these and quite frankly decided to ignore them. Do we
want to work around old/broken libraries? Where do we write the line?

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 3/3] tools: Enable warnings for more binaries/libs
Posted by Peter Krempa 7 years, 5 months ago 
On Thu, Nov 23, 2017 at 16:49:45 +0100, Michal Privoznik wrote:
> On 11/23/2017 03:10 PM, Peter Krempa wrote:
> > On Mon, Nov 20, 2017 at 17:36:54 +0100, Erik Skultety wrote:
> >> On Thu, Nov 16, 2017 at 02:49:29PM +0100, Michal Privoznik wrote:
> >>> Because WARN_CFLAGS and COVERAGE_CFLAGS are not set globally, we
> >>> rely on each binary built to include WARN_CFLAGS/COVERAGE_CFLAGS.
> >>> But it is easy to forget those - e.g. libvirt_shell.la. However,
> >>> don't enable WARN_FLAGS (i.e. don't include AM_CFLAGS) for
> >>> wireshark plugin - parts of that code are generated and trigger
> >>> some warnings.
> >>>
> >>> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> >>> ---
> >>>  tools/Makefile.am | 17 +++++++++--------
> >>>  1 file changed, 9 insertions(+), 8 deletions(-)
> > 
> > This commit broke build on OSX [1] since apparently readline has
> > different definition for some internal variables:
> > 
> > vsh.c:2903:22: error: assigning to 'char *' from 'const char *' discards qualifiers [-Werror,-Wincompatible-pointer-types-discards-qualifiers]
> > 
> >     rl_readline_name = ctl->name;
> > 
> >                      ^ ~~~~~~~~~
> > 
> > vsh.c:2908:36: error: assigning to 'char *' from 'const char [16]' discards qualifiers [-Werror,-Wincompatible-pointer-types-discards-qualifiers]
> > 
> >     rl_basic_word_break_characters = " \t\n\\`@$><=;|&{(";
> > 
> >                                    ^ ~~~~~~~~~~~~~~~~~~~~
> > 
> > 2 errors generated.
> > 
> > https://travis-ci.org/libvirt/libvirt/jobs/305251968
> > 
> 
> Right. I've seen these and quite frankly decided to ignore them. Do we
> want to work around old/broken libraries? Where do we write the line?

Well, we were ignoring the errors in the build system and then you chose
to change that. So if you want to enforce the werror flag, you should
fix the fallout. The other option is to stop enforcing werror in this
case as we did until now.

The problem is that despite 'old/broken' libraries, you made it
unbuildable.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Patchew 2.1-devel-8c64711

© 2016 - 2025 Red Hat, Inc.