7:49 a.m.
*** 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
7:49 a.m.
New subject: [libvirt] [PATCH 1/3] vsh: Make self-test more robust
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(a)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
3:25 a.m.
New subject: [libvirt] [PATCH 1/3] vsh: Make self-test more robust
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(a)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(a)redhat.com>
6:22 a.m.
New subject: [libvirt] [PATCH 1/3] vsh: Make self-test more robust
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(a)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
7:49 a.m.
New subject: [libvirt] [PATCH 2/3] tools: Set CFLAGS for wireshark properly
We want to set CFLAGS not CPPFLAGS.
Signed-off-by: Michal Privoznik <mprivozn(a)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
10:51 a.m.
New subject: [libvirt] [PATCH 2/3] tools: Set CFLAGS for wireshark properly
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(a)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(a)redhat.com>
7:49 a.m.
New subject: [libvirt] [PATCH 3/3] tools: Enable warnings for more binaries/libs
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(a)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
10:36 a.m.
New subject: [libvirt] [PATCH 3/3] tools: Enable warnings for more binaries/libs
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(a)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(a)redhat.com>
8:10 a.m.
New subject: [libvirt] [PATCH 3/3] tools: Enable warnings for more binaries/libs
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(a)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
9:49 a.m.
New subject: [libvirt] [PATCH 3/3] tools: Enable warnings for more binaries/libs
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(a)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
9:52 a.m.
New subject: [libvirt] [PATCH 3/3] tools: Enable warnings for more binaries/libs
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(a)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.
2590
days inactive
2597
days old
10 comments
3 participants
participants (3)
-
Erik Skultety -
Michal Privoznik -
Peter Krempa