[libvirt] [PATCH 0/3] Couple of improvements
*** BLURB HERE *** Michal Privoznik (3): vsh: Make self-test more robust tools: Set CFLAGS for wireshark properly tools: Enable warnings for more binaries/libs tools/Makefile.am | 19 ++++++++++--------- tools/vsh.c | 29 ++++++++++++++++++++++------- 2 files changed, 32 insertions(+), 16 deletions(-) -- 2.13.6
There are couple of limitations when it comes to option types and flags for the options. For instance, VSH_OT_STRING cannot have VSH_OFLAG_REQ set (commit c7543a728). For some reason this is checked in vshCmddefHelp() but not in vshCmddefCheckInternals(). Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tools/vsh.c | 29 ++++++++++++++++++++++------- 1 file changed, 22 insertions(+), 7 deletions(-) diff --git a/tools/vsh.c b/tools/vsh.c index 10a65c39f..75568353d 100644 --- a/tools/vsh.c +++ b/tools/vsh.c @@ -363,12 +363,15 @@ vshCmddefCheckInternals(const vshCmdDef *cmd) if (i > 63) return -1; /* too many options */ - if (opt->type == VSH_OT_BOOL) { + + switch (opt->type) { + case VSH_OT_STRING: + case VSH_OT_BOOL: if (opt->flags & VSH_OFLAG_REQ) return -1; /* bool options can't be mandatory */ - continue; - } - if (opt->type == VSH_OT_ALIAS) { + break; + + case VSH_OT_ALIAS: { size_t j; char *name = (char *)opt->help; /* cast away const */ char *p; @@ -391,10 +394,22 @@ vshCmddefCheckInternals(const vshCmdDef *cmd) } if (!cmd->opts[j].name) return -1; /* alias option must map to a later option name */ - continue; } - if (opt->type == VSH_OT_ARGV && cmd->opts[i + 1].name) - return -1; /* argv option must be listed last */ + break; + case VSH_OT_ARGV: + if (cmd->opts[i + 1].name) + return -1; /* argv option must be listed last */ + break; + + case VSH_OT_DATA: + if (!(opt->flags & VSH_OFLAG_REQ)) + return -1; /* OT_DATA should always be required. */ + break; + + case VSH_OT_INT: + /* nada */ + break; + } } return 0; } -- 2.13.6
On Thu, Nov 16, 2017 at 02:49:27PM +0100, Michal Privoznik wrote:
There are couple of limitations when it comes to option types and flags for the options. For instance, VSH_OT_STRING cannot have VSH_OFLAG_REQ set (commit c7543a728). For some reason this is checked in vshCmddefHelp() but not in vshCmddefCheckInternals().
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> ---
[...]
- if (opt->type == VSH_OT_ARGV && cmd->opts[i + 1].name) - return -1; /* argv option must be listed last */ + break; + case VSH_OT_ARGV: + if (cmd->opts[i + 1].name) + return -1; /* argv option must be listed last */ + break; + + case VSH_OT_DATA: + if (!(opt->flags & VSH_OFLAG_REQ)) + return -1; /* OT_DATA should always be required. */
This got me thinking a bit, since we're going to do the checking here, is there a need for performing the same check within vshCmddefHelp too? My reasoning is that virsh-self-test is part of the test suite run at make check. Not a deal breaker though, just thinking out loud.
+ break; + + case VSH_OT_INT: + /* nada */
please don't...
+ break; + }
Reviewed-by: Erik Skultety <eskultet@redhat.com>
On 11/21/2017 10:25 AM, Erik Skultety wrote:
On Thu, Nov 16, 2017 at 02:49:27PM +0100, Michal Privoznik wrote:
There are couple of limitations when it comes to option types and flags for the options. For instance, VSH_OT_STRING cannot have VSH_OFLAG_REQ set (commit c7543a728). For some reason this is checked in vshCmddefHelp() but not in vshCmddefCheckInternals().
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> ---
[...]
- if (opt->type == VSH_OT_ARGV && cmd->opts[i + 1].name) - return -1; /* argv option must be listed last */ + break; + case VSH_OT_ARGV: + if (cmd->opts[i + 1].name) + return -1; /* argv option must be listed last */ + break; + + case VSH_OT_DATA: + if (!(opt->flags & VSH_OFLAG_REQ)) + return -1; /* OT_DATA should always be required. */
This got me thinking a bit, since we're going to do the checking here, is there a need for performing the same check within vshCmddefHelp too? My reasoning is that virsh-self-test is part of the test suite run at make check. Not a deal breaker though, just thinking out loud.
Yeah, it probably doesn't. But frankly, I don't know why we have any check at vshCmdDefHelp in the first place. Maybe it used to be a test case? Like we ran all the commands with --help? Anyway, it doesn't make sense now so I'll remove it.
+ break; + + case VSH_OT_INT: + /* nada */
please don't...
¿Por qué no? Michal
We want to set CFLAGS not CPPFLAGS. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tools/Makefile.am | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/Makefile.am b/tools/Makefile.am index 7513a73ac..a844dcbbc 100644 --- a/tools/Makefile.am +++ b/tools/Makefile.am @@ -427,7 +427,7 @@ EXTRA_DIST += \ if WITH_WIRESHARK_DISSECTOR ws_plugin_LTLIBRARIES = wireshark/src/libvirt.la -wireshark_src_libvirt_la_CPPFLAGS = \ +wireshark_src_libvirt_la_CFLAGS = \ -I wireshark/src $(WIRESHARK_DISSECTOR_CFLAGS) wireshark_src_libvirt_la_LDFLAGS = -avoid-version -module nodist_wireshark_src_libvirt_la_SOURCES = wireshark/src/plugin.c -- 2.13.6
On Thu, Nov 16, 2017 at 02:49:28PM +0100, Michal Privoznik wrote:
We want to set CFLAGS not CPPFLAGS.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tools/Makefile.am | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/Makefile.am b/tools/Makefile.am index 7513a73ac..a844dcbbc 100644 --- a/tools/Makefile.am +++ b/tools/Makefile.am @@ -427,7 +427,7 @@ EXTRA_DIST += \ if WITH_WIRESHARK_DISSECTOR
ws_plugin_LTLIBRARIES = wireshark/src/libvirt.la -wireshark_src_libvirt_la_CPPFLAGS = \
CPPFLAGS are flags for preprocessor IIUC but judging from the variable's content == other CFLAGS, I think this is OK while also being a "no change". Reviewed-by: Erik Skultety <eskultet@redhat.com>
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
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>
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
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.
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
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.
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.
participants (3)
-
Erik Skultety -
Michal Privoznik -
Peter Krempa