11:52 a.m.
As always with new GCC major releases, we've tickled some new
warnings. What's nice is that two of them identified genuine
bugs in our code.
I would push this as a build-breaker fix, but I wanted some
visibility on the fourth patch before doing that, as it was
not the usual type of quick fix.
Daniel P. Berrange (4):
Use explicit boolean comparison in OOM check
libxl: fix empty string check for channel path
qemu: add missing break in qemuDomainDeviceCalculatePCIConnectFlags
Add ATTRIBUTE_FALLTHROUGH for switch cases without break
src/conf/domain_conf.c | 7 +++++++
src/conf/network_conf.c | 3 ++-
src/internal.h | 8 ++++++++
src/libxl/libxl_domain.c | 2 +-
src/lxc/lxc_container.c | 2 +-
src/network/bridge_driver.c | 6 ++++++
src/qemu/qemu_domain_address.c | 1 +
src/util/viralloc.c | 2 +-
tools/virsh-edit.c | 2 +-
9 files changed, 28 insertions(+), 5 deletions(-)
--
2.9.3
11:52 a.m.
New subject: [libvirt] [PATCH 1/4] Use explicit boolean comparison in OOM check
GCC 7 gets upset by
if (!tmp && (size * count))
warning
util/viralloc.c: In function 'virReallocN':
util/viralloc.c:246:23: error: '*' in boolean context, suggest
'&&' instead [-Werror=int-in-bool-context]
if (!tmp && (size * count)) {
~~~~~~^~~~~~~~
Keep it happy by adding != 0 to the right hand expression
so it realizes we really are wanting to treat the result
of the arithmetic expression as a boolean
Signed-off-by: Daniel P. Berrange <berrange(a)redhat.com>
---
src/util/viralloc.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/util/viralloc.c b/src/util/viralloc.c
index 812aa5b..81f99d9 100644
--- a/src/util/viralloc.c
+++ b/src/util/viralloc.c
@@ -243,7 +243,7 @@ int virReallocN(void *ptrptr,
return -1;
}
tmp = realloc(*(void**)ptrptr, size * count);
- if (!tmp && (size * count)) {
+ if (!tmp && ((size * count) != 0)) {
if (report)
virReportOOMErrorFull(domcode, filename, funcname, linenr);
return -1;
--
2.9.3
1:48 p.m.
New subject: [libvirt] [PATCH 1/4] Use explicit boolean comparison in OOM check
On 02/22/2017 12:52 PM, Daniel P. Berrange wrote:
GCC 7 gets upset by
if (!tmp && (size * count))
warning
util/viralloc.c: In function 'virReallocN':
util/viralloc.c:246:23: error: '*' in boolean context, suggest
'&&' instead [-Werror=int-in-bool-context]
if (!tmp && (size * count)) {
~~~~~~^~~~~~~~
Keep it happy by adding != 0 to the right hand expression
so it realizes we really are wanting to treat the result
of the arithmetic expression as a boolean
ACK.
2:41 a.m.
New subject: [libvirt] [PATCH 1/4] Use explicit boolean comparison in OOM check
On 02/22/2017 06:52 PM, Daniel P. Berrange wrote:
GCC 7 gets upset by
if (!tmp && (size * count))
warning
util/viralloc.c: In function 'virReallocN':
util/viralloc.c:246:23: error: '*' in boolean context, suggest
'&&' instead [-Werror=int-in-bool-context]
if (!tmp && (size * count)) {
~~~~~~^~~~~~~~
Keep it happy by adding != 0 to the right hand expression
so it realizes we really are wanting to treat the result
of the arithmetic expression as a boolean
Signed-off-by: Daniel P. Berrange <berrange(a)redhat.com>
---
src/util/viralloc.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/util/viralloc.c b/src/util/viralloc.c
index 812aa5b..81f99d9 100644
--- a/src/util/viralloc.c
+++ b/src/util/viralloc.c
@@ -243,7 +243,7 @@ int virReallocN(void *ptrptr,
return -1;
}
tmp = realloc(*(void**)ptrptr, size * count);
- if (!tmp && (size * count)) {
+ if (!tmp && ((size * count) != 0)) {
if (report)
virReportOOMErrorFull(domcode, filename, funcname, linenr);
return -1;
This is just stupid. I mean the warning, not your fix.
Michal
3:27 a.m.
New subject: [libvirt] [PATCH 1/4] Use explicit boolean comparison in OOM check
On Thu, Feb 23, 2017 at 09:41:12AM +0100, Michal Privoznik wrote:
On 02/22/2017 06:52 PM, Daniel P. Berrange wrote:
> GCC 7 gets upset by
>
> if (!tmp && (size * count))
>
> warning
>
> util/viralloc.c: In function 'virReallocN':
> util/viralloc.c:246:23: error: '*' in boolean context, suggest
'&&' instead [-Werror=int-in-bool-context]
> if (!tmp && (size * count)) {
> ~~~~~~^~~~~~~~
>
> Keep it happy by adding != 0 to the right hand expression
> so it realizes we really are wanting to treat the result
> of the arithmetic expression as a boolean
>
> Signed-off-by: Daniel P. Berrange <berrange(a)redhat.com>
> ---
> src/util/viralloc.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/src/util/viralloc.c b/src/util/viralloc.c
> index 812aa5b..81f99d9 100644
> --- a/src/util/viralloc.c
> +++ b/src/util/viralloc.c
> @@ -243,7 +243,7 @@ int virReallocN(void *ptrptr,
> return -1;
> }
> tmp = realloc(*(void**)ptrptr, size * count);
> - if (!tmp && (size * count)) {
> + if (!tmp && ((size * count) != 0)) {
> if (report)
> virReportOOMErrorFull(domcode, filename, funcname, linenr);
> return -1;
>
This is just stupid. I mean the warning, not your fix.
It is a warning that is certainly going to trigger a non-negligible
number of false positives across various codebases, but I don't think
it is stupid. The pattern it is looking for here with mixed integer
and boolean operators has been a repeated source of bugs in software
and a number of them have resulted in CVEs before when they've been
mis-handling untrusted input validation or breaking crypto algorithms,
etc.
Regards,
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://entangle-photo.org -o- http://search.cpan.org/~danberr/ :|
11:52 a.m.
New subject: [libvirt] [PATCH 2/4] libxl: fix empty string check for channel path
The libxl code was checking that a 'char *' was != '\0', instead
of checking the first element in the string
Signed-off-by: Daniel P. Berrange <berrange(a)redhat.com>
---
src/libxl/libxl_domain.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c
index 57ec661..ea28c93 100644
--- a/src/libxl/libxl_domain.c
+++ b/src/libxl/libxl_domain.c
@@ -1121,7 +1121,7 @@ libxlDomainCreateChannelPTY(virDomainDefPtr def, libxl_ctx *ctx)
&channelinfo);
if (!ret && channelinfo.u.pty.path &&
- channelinfo.u.pty.path != '\0') {
+ *channelinfo.u.pty.path != '\0') {
VIR_FREE(chr->source->data.file.path);
ignore_value(VIR_STRDUP(chr->source->data.file.path,
channelinfo.u.pty.path));
--
2.9.3
12:35 p.m.
New subject: [libvirt] [PATCH 2/4] libxl: fix empty string check for channel path
On 02/22/2017 12:52 PM, Daniel P. Berrange wrote:
The libxl code was checking that a 'char *' was !=
'\0', instead
of checking the first element in the string
Signed-off-by: Daniel P. Berrange <berrange(a)redhat.com>
---
src/libxl/libxl_domain.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c
index 57ec661..ea28c93 100644
--- a/src/libxl/libxl_domain.c
+++ b/src/libxl/libxl_domain.c
@@ -1121,7 +1121,7 @@ libxlDomainCreateChannelPTY(virDomainDefPtr def, libxl_ctx *ctx)
&channelinfo);
if (!ret && channelinfo.u.pty.path &&
- channelinfo.u.pty.path != '\0') {
+ *channelinfo.u.pty.path != '\0') {
VIR_FREE(chr->source->data.file.path);
ignore_value(VIR_STRDUP(chr->source->data.file.path,
channelinfo.u.pty.path));
ACK.
11:52 a.m.
New subject: [libvirt] [PATCH 3/4] qemu: add missing break in qemuDomainDeviceCalculatePCIConnectFlags
One of the conditions in qemuDomainDeviceCalculatePCIConnectFlags
was missing a break that could result it in falling through to
an incorrect codepath.
Signed-off-by: Daniel P. Berrange <berrange(a)redhat.com>
---
src/qemu/qemu_domain_address.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c
index 5b75044..27ca010 100644
--- a/src/qemu/qemu_domain_address.c
+++ b/src/qemu/qemu_domain_address.c
@@ -553,6 +553,7 @@ qemuDomainDeviceCalculatePCIConnectFlags(virDomainDeviceDefPtr dev,
return pciFlags;
}
}
+ break;
case VIR_DOMAIN_DEVICE_FS:
/* the only type of filesystem so far is virtio-9p-pci */
--
2.9.3
12:27 p.m.
New subject: [libvirt] [PATCH 3/4] qemu: add missing break in qemuDomainDeviceCalculatePCIConnectFlags
On Wed, 2017-02-22 at 17:52 +0000, Daniel P. Berrange wrote:
@@ -553,6 +553,7 @@
qemuDomainDeviceCalculatePCIConnectFlags(virDomainDeviceDefPtr dev,
return pciFlags;
}
}
+ break;
case VIR_DOMAIN_DEVICE_FS:
/* the only type of filesystem so far is virtio-9p-pci */
I don't get this: all switches, whether top-level with
respect to the function or not, provide full coverage of
the respective enum, and inside those switches all cases
return explicitly, so none of them can really fall
through.
How is it that only this one is bothering the compiler?
--
Andrea Bolognani / Red Hat / Virtualization
1:44 p.m.
New subject: [libvirt] [PATCH 3/4] qemu: add missing break in qemuDomainDeviceCalculatePCIConnectFlags
On 02/22/2017 12:52 PM, Daniel P. Berrange wrote:
One of the conditions in qemuDomainDeviceCalculatePCIConnectFlags
was missing a break that could result it in falling through to
an incorrect codepath.
Actually that's not true. Every codepath of the preceding case ends with
a "return blah". This is true for the entire function - every case of
every switch in the function ends with "return blah". The entire purpose
of the function is to determine the flags value, and there are no
resources that need cleaning up before returning, so as soon as the
value is determined, it immediately returns.
I suppose it could be rewritten to change all of those into "ret = blah;
break;", then "return ret;" at the end, but it seemed safer to return
immediately than to trust that no new code would be added later in the
function (and also it's more compact)
I wonder if this is just a more extreme case of the logic in whatever
check necessitated that I add an extra "return 0" at the very end of the
function. (that happens even in gcc 6.x; at an earlier point when the
function was simpler, that wasn't needed, but after some additions it
started producing the "control reaches end of function that requires a
return value" or whatever that warning is, and the only way to eliminate
it was with the extra return 0.)
If adding the break shuts up the warning, then I guess ACK, but it would
probably be better if 1) gcc fixed their incorrect warning, or 2) we
switched the entire function to use the less-compact "ret = blah;
break;" style instead of returning directly, so there wasn't a single
stray break sitting in the middle.
Signed-off-by: Daniel P. Berrange <berrange(a)redhat.com>
---
src/qemu/qemu_domain_address.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c
index 5b75044..27ca010 100644
--- a/src/qemu/qemu_domain_address.c
+++ b/src/qemu/qemu_domain_address.c
@@ -553,6 +553,7 @@ qemuDomainDeviceCalculatePCIConnectFlags(virDomainDeviceDefPtr dev,
return pciFlags;
}
}
+ break;
case VIR_DOMAIN_DEVICE_FS:
/* the only type of filesystem so far is virtio-9p-pci */
2:19 p.m.
New subject: [libvirt] [PATCH 3/4] qemu: add missing break in qemuDomainDeviceCalculatePCIConnectFlags
On Wed, Feb 22, 2017 at 02:44:01PM -0500, Laine Stump wrote:
On 02/22/2017 12:52 PM, Daniel P. Berrange wrote:
> One of the conditions in qemuDomainDeviceCalculatePCIConnectFlags
> was missing a break that could result it in falling through to
> an incorrect codepath.
Actually that's not true. Every codepath of the preceding case ends with
a "return blah". This is true for the entire function - every case of
every switch in the function ends with "return blah". The entire purpose
of the function is to determine the flags value, and there are no
resources that need cleaning up before returning, so as soon as the
value is determined, it immediately returns.
I suppose it could be rewritten to change all of those into "ret = blah;
break;", then "return ret;" at the end, but it seemed safer to return
immediately than to trust that no new code would be added later in the
function (and also it's more compact)
I wonder if this is just a more extreme case of the logic in whatever
check necessitated that I add an extra "return 0" at the very end of the
function. (that happens even in gcc 6.x; at an earlier point when the
function was simpler, that wasn't needed, but after some additions it
started producing the "control reaches end of function that requires a
return value" or whatever that warning is, and the only way to eliminate
it was with the extra return 0.)
If adding the break shuts up the warning, then I guess ACK, but it would
probably be better if 1) gcc fixed their incorrect warning, or 2) we
switched the entire function to use the less-compact "ret = blah;
break;" style instead of returning directly, so there wasn't a single
stray break sitting in the middle.
I would say NACK since 1) is the correct option (at least for now),
there is no reason for adding more lines of code that don't make sense
just because of a compiler version that was not released yet, or does
not even have a release plan yet.
>
> Signed-off-by: Daniel P. Berrange <berrange(a)redhat.com>
> ---
> src/qemu/qemu_domain_address.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c
> index 5b75044..27ca010 100644
> --- a/src/qemu/qemu_domain_address.c
> +++ b/src/qemu/qemu_domain_address.c
> @@ -553,6 +553,7 @@ qemuDomainDeviceCalculatePCIConnectFlags(virDomainDeviceDefPtr
dev,
> return pciFlags;
> }
> }
> + break;
>
> case VIR_DOMAIN_DEVICE_FS:
> /* the only type of filesystem so far is virtio-9p-pci */
--
libvir-list mailing list
libvir-list(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
3:48 a.m.
New subject: [libvirt] [PATCH 3/4] qemu: add missing break in qemuDomainDeviceCalculatePCIConnectFlags
On Wed, Feb 22, 2017 at 09:19:15PM +0100, Martin Kletzander wrote:
On Wed, Feb 22, 2017 at 02:44:01PM -0500, Laine Stump wrote:
> On 02/22/2017 12:52 PM, Daniel P. Berrange wrote:
> > One of the conditions in qemuDomainDeviceCalculatePCIConnectFlags
> > was missing a break that could result it in falling through to
> > an incorrect codepath.
>
> Actually that's not true. Every codepath of the preceding case ends with
> a "return blah". This is true for the entire function - every case of
> every switch in the function ends with "return blah". The entire purpose
> of the function is to determine the flags value, and there are no
> resources that need cleaning up before returning, so as soon as the
> value is determined, it immediately returns.
>
> I suppose it could be rewritten to change all of those into "ret = blah;
> break;", then "return ret;" at the end, but it seemed safer to
return
> immediately than to trust that no new code would be added later in the
> function (and also it's more compact)
>
> I wonder if this is just a more extreme case of the logic in whatever
> check necessitated that I add an extra "return 0" at the very end of the
> function. (that happens even in gcc 6.x; at an earlier point when the
> function was simpler, that wasn't needed, but after some additions it
> started producing the "control reaches end of function that requires a
> return value" or whatever that warning is, and the only way to eliminate
> it was with the extra return 0.)
>
> If adding the break shuts up the warning, then I guess ACK, but it would
> probably be better if 1) gcc fixed their incorrect warning, or 2) we
> switched the entire function to use the less-compact "ret = blah;
> break;" style instead of returning directly, so there wasn't a single
> stray break sitting in the middle.
>
I would say NACK since 1) is the correct option (at least for now),
there is no reason for adding more lines of code that don't make sense
just because of a compiler version that was not released yet, or does
not even have a release plan yet.
GCC 7 *is* released - and has even had a bug fix release too, so ignoring
this is not an option. In any case, as Eric mentions this is a genuine
bug in our code since we can fall out of the inner switch if the input
variable contains a value that doesn't map to an named enum value.
Regards,
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://entangle-photo.org -o- http://search.cpan.org/~danberr/ :|
4:38 a.m.
New subject: [libvirt] [PATCH 3/4] qemu: add missing break in qemuDomainDeviceCalculatePCIConnectFlags
On Thu, Feb 23, 2017 at 09:48:48AM +0000, Daniel P. Berrange wrote:
On Wed, Feb 22, 2017 at 09:19:15PM +0100, Martin Kletzander wrote:
> On Wed, Feb 22, 2017 at 02:44:01PM -0500, Laine Stump wrote:
> > On 02/22/2017 12:52 PM, Daniel P. Berrange wrote:
> > > One of the conditions in qemuDomainDeviceCalculatePCIConnectFlags
> > > was missing a break that could result it in falling through to
> > > an incorrect codepath.
> >
> > Actually that's not true. Every codepath of the preceding case ends with
> > a "return blah". This is true for the entire function - every case
of
> > every switch in the function ends with "return blah". The entire
purpose
> > of the function is to determine the flags value, and there are no
> > resources that need cleaning up before returning, so as soon as the
> > value is determined, it immediately returns.
> >
> > I suppose it could be rewritten to change all of those into "ret = blah;
> > break;", then "return ret;" at the end, but it seemed safer to
return
> > immediately than to trust that no new code would be added later in the
> > function (and also it's more compact)
> >
> > I wonder if this is just a more extreme case of the logic in whatever
> > check necessitated that I add an extra "return 0" at the very end of
the
> > function. (that happens even in gcc 6.x; at an earlier point when the
> > function was simpler, that wasn't needed, but after some additions it
> > started producing the "control reaches end of function that requires a
> > return value" or whatever that warning is, and the only way to eliminate
> > it was with the extra return 0.)
> >
> > If adding the break shuts up the warning, then I guess ACK, but it would
> > probably be better if 1) gcc fixed their incorrect warning, or 2) we
> > switched the entire function to use the less-compact "ret = blah;
> > break;" style instead of returning directly, so there wasn't a single
> > stray break sitting in the middle.
> >
>
> I would say NACK since 1) is the correct option (at least for now),
> there is no reason for adding more lines of code that don't make sense
> just because of a compiler version that was not released yet, or does
> not even have a release plan yet.
GCC 7 *is* released - and has even had a bug fix release too, so ignoring
this is not an option. In any case, as Eric mentions this is a genuine
bug in our code since we can fall out of the inner switch if the input
variable contains a value that doesn't map to an named enum value.
Where did you get the package/tarball? I don't see anything in the
release page [1]. On the other hand, when I checked it yesterday, I
looked and the development timeline [2] and I thought it's 2016
apparently because when I see the dates now it makes sense that the
release should be around the corner. Anyway, even if they did not
update the release page, on snapshot ftp [3] there is not even a release
candidate.
I remember others not being happy when we were doing workarounds for
packages that downstream distros just decided to package out of VCS or
snapshots. I don't feel it's right either and I thought you're on that
side as well. Anyway, if it really was released, I am OK with this
going in.
Martin
[1] https://gcc.gnu.org/releases.html
[2] https://gcc.gnu.org/develop.html#timeline
[3] ftp://gcc.gnu.org/pub/gcc/snapshots/
Regards,
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://entangle-photo.org -o- http://search.cpan.org/~danberr/ :|
4:46 a.m.
New subject: [libvirt] [PATCH 3/4] qemu: add missing break in qemuDomainDeviceCalculatePCIConnectFlags
On Thu, Feb 23, 2017 at 11:38:40AM +0100, Martin Kletzander wrote:
On Thu, Feb 23, 2017 at 09:48:48AM +0000, Daniel P. Berrange wrote:
> On Wed, Feb 22, 2017 at 09:19:15PM +0100, Martin Kletzander wrote:
> > On Wed, Feb 22, 2017 at 02:44:01PM -0500, Laine Stump wrote:
> > > On 02/22/2017 12:52 PM, Daniel P. Berrange wrote:
> > > > One of the conditions in qemuDomainDeviceCalculatePCIConnectFlags
> > > > was missing a break that could result it in falling through to
> > > > an incorrect codepath.
> > >
> > > Actually that's not true. Every codepath of the preceding case ends
with
> > > a "return blah". This is true for the entire function - every
case of
> > > every switch in the function ends with "return blah". The entire
purpose
> > > of the function is to determine the flags value, and there are no
> > > resources that need cleaning up before returning, so as soon as the
> > > value is determined, it immediately returns.
> > >
> > > I suppose it could be rewritten to change all of those into "ret =
blah;
> > > break;", then "return ret;" at the end, but it seemed safer
to return
> > > immediately than to trust that no new code would be added later in the
> > > function (and also it's more compact)
> > >
> > > I wonder if this is just a more extreme case of the logic in whatever
> > > check necessitated that I add an extra "return 0" at the very
end of the
> > > function. (that happens even in gcc 6.x; at an earlier point when the
> > > function was simpler, that wasn't needed, but after some additions it
> > > started producing the "control reaches end of function that requires
a
> > > return value" or whatever that warning is, and the only way to
eliminate
> > > it was with the extra return 0.)
> > >
> > > If adding the break shuts up the warning, then I guess ACK, but it would
> > > probably be better if 1) gcc fixed their incorrect warning, or 2) we
> > > switched the entire function to use the less-compact "ret = blah;
> > > break;" style instead of returning directly, so there wasn't a
single
> > > stray break sitting in the middle.
> > >
> >
> > I would say NACK since 1) is the correct option (at least for now),
> > there is no reason for adding more lines of code that don't make sense
> > just because of a compiler version that was not released yet, or does
> > not even have a release plan yet.
>
> GCC 7 *is* released - and has even had a bug fix release too, so ignoring
> this is not an option. In any case, as Eric mentions this is a genuine
> bug in our code since we can fall out of the inner switch if the input
> variable contains a value that doesn't map to an named enum value.
>
Where did you get the package/tarball? I don't see anything in the
release page [1]. On the other hand, when I checked it yesterday, I
looked and the development timeline [2] and I thought it's 2016
apparently because when I see the dates now it makes sense that the
release should be around the corner. Anyway, even if they did not
update the release page, on snapshot ftp [3] there is not even a release
candidate.
I didn't use any tarball - just what's in Fedora which is
gcc-7.0.1-0.9.fc26.x86_64
Fedora dist-git says the tarball is gcc-7.0.1-20170219.tar.bz2
Odd that its not on the download page though as that's a clearly a
release version number, not a git snapshot or pre-release version.
I remember others not being happy when we were doing workarounds for
packages that downstream distros just decided to package out of VCS or
snapshots. I don't feel it's right either and I thought you're on that
side as well. Anyway, if it really was released, I am OK with this
going in.
Regardless of whether its a release or pre-release, this is a clear
bug in the code that needs fixing - its just not a workaround for a
compiler. As such I've pushed this series.
Regards,
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://entangle-photo.org -o- http://search.cpan.org/~danberr/ :|
6:15 a.m.
New subject: [libvirt] [PATCH 3/4] qemu: add missing break in qemuDomainDeviceCalculatePCIConnectFlags
On Thu, Feb 23, 2017 at 10:46:14AM +0000, Daniel P. Berrange wrote:
On Thu, Feb 23, 2017 at 11:38:40AM +0100, Martin Kletzander wrote:
> On Thu, Feb 23, 2017 at 09:48:48AM +0000, Daniel P. Berrange wrote:
> > On Wed, Feb 22, 2017 at 09:19:15PM +0100, Martin Kletzander wrote:
> > > On Wed, Feb 22, 2017 at 02:44:01PM -0500, Laine Stump wrote:
> > > > On 02/22/2017 12:52 PM, Daniel P. Berrange wrote:
> > > > > One of the conditions in
qemuDomainDeviceCalculatePCIConnectFlags
> > > > > was missing a break that could result it in falling through to
> > > > > an incorrect codepath.
> > > >
> > > > Actually that's not true. Every codepath of the preceding case
ends with
> > > > a "return blah". This is true for the entire function -
every case of
> > > > every switch in the function ends with "return blah". The
entire purpose
> > > > of the function is to determine the flags value, and there are no
> > > > resources that need cleaning up before returning, so as soon as the
> > > > value is determined, it immediately returns.
> > > >
> > > > I suppose it could be rewritten to change all of those into "ret
= blah;
> > > > break;", then "return ret;" at the end, but it seemed
safer to return
> > > > immediately than to trust that no new code would be added later in
the
> > > > function (and also it's more compact)
> > > >
> > > > I wonder if this is just a more extreme case of the logic in whatever
> > > > check necessitated that I add an extra "return 0" at the
very end of the
> > > > function. (that happens even in gcc 6.x; at an earlier point when the
> > > > function was simpler, that wasn't needed, but after some additions
it
> > > > started producing the "control reaches end of function that
requires a
> > > > return value" or whatever that warning is, and the only way to
eliminate
> > > > it was with the extra return 0.)
> > > >
> > > > If adding the break shuts up the warning, then I guess ACK, but it
would
> > > > probably be better if 1) gcc fixed their incorrect warning, or 2) we
> > > > switched the entire function to use the less-compact "ret =
blah;
> > > > break;" style instead of returning directly, so there wasn't
a single
> > > > stray break sitting in the middle.
> > > >
> > >
> > > I would say NACK since 1) is the correct option (at least for now),
> > > there is no reason for adding more lines of code that don't make sense
> > > just because of a compiler version that was not released yet, or does
> > > not even have a release plan yet.
> >
> > GCC 7 *is* released - and has even had a bug fix release too, so ignoring
> > this is not an option. In any case, as Eric mentions this is a genuine
> > bug in our code since we can fall out of the inner switch if the input
> > variable contains a value that doesn't map to an named enum value.
> >
>
> Where did you get the package/tarball? I don't see anything in the
> release page [1]. On the other hand, when I checked it yesterday, I
> looked and the development timeline [2] and I thought it's 2016
> apparently because when I see the dates now it makes sense that the
> release should be around the corner. Anyway, even if they did not
> update the release page, on snapshot ftp [3] there is not even a release
> candidate.
I didn't use any tarball - just what's in Fedora which is
gcc-7.0.1-0.9.fc26.x86_64
Fedora dist-git says the tarball is gcc-7.0.1-20170219.tar.bz2
Odd that its not on the download page though as that's a clearly a
release version number, not a git snapshot or pre-release version.
The timestamp indicates it's just a snapshot. GCC doesn't do x.0
releases since gcc-5, I believe, so unless it's 7.1 it's not an official
release.
> I remember others not being happy when we were doing workarounds
for
> packages that downstream distros just decided to package out of VCS or
> snapshots. I don't feel it's right either and I thought you're on that
> side as well. Anyway, if it really was released, I am OK with this
> going in.
Regardless of whether its a release or pre-release, this is a clear
bug in the code that needs fixing - its just not a workaround for a
compiler. As such I've pushed this series.
As Laine said as well, there is no bug in the code. All the codepaths
in that switch case end with return. And it *is* clearly a bug in the
compiler as there are other cases (e.g. VIR_DOMAIN_CONTROLLER_TYPE_SCSI)
that behave the same, there is no break after it and the compiler is
clearly OK with all the other ones.
Regards,
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://entangle-photo.org -o- http://search.cpan.org/~danberr/ :|
--
libvir-list mailing list
libvir-list(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
6:25 a.m.
New subject: [libvirt] [PATCH 3/4] qemu: add missing break in qemuDomainDeviceCalculatePCIConnectFlags
On Thu, Feb 23, 2017 at 01:15:50PM +0100, Martin Kletzander wrote:
On Thu, Feb 23, 2017 at 10:46:14AM +0000, Daniel P. Berrange wrote:
> On Thu, Feb 23, 2017 at 11:38:40AM +0100, Martin Kletzander wrote:
> > On Thu, Feb 23, 2017 at 09:48:48AM +0000, Daniel P. Berrange wrote:
> > > On Wed, Feb 22, 2017 at 09:19:15PM +0100, Martin Kletzander wrote:
> > > > On Wed, Feb 22, 2017 at 02:44:01PM -0500, Laine Stump wrote:
> > > > > On 02/22/2017 12:52 PM, Daniel P. Berrange wrote:
> > > > > > One of the conditions in
qemuDomainDeviceCalculatePCIConnectFlags
> > > > > > was missing a break that could result it in falling through
to
> > > > > > an incorrect codepath.
> > > > >
> > > > > Actually that's not true. Every codepath of the preceding
case ends with
> > > > > a "return blah". This is true for the entire function
- every case of
> > > > > every switch in the function ends with "return blah".
The entire purpose
> > > > > of the function is to determine the flags value, and there are
no
> > > > > resources that need cleaning up before returning, so as soon as
the
> > > > > value is determined, it immediately returns.
> > > > >
> > > > > I suppose it could be rewritten to change all of those into
"ret = blah;
> > > > > break;", then "return ret;" at the end, but it
seemed safer to return
> > > > > immediately than to trust that no new code would be added later
in the
> > > > > function (and also it's more compact)
> > > > >
> > > > > I wonder if this is just a more extreme case of the logic in
whatever
> > > > > check necessitated that I add an extra "return 0" at
the very end of the
> > > > > function. (that happens even in gcc 6.x; at an earlier point
when the
> > > > > function was simpler, that wasn't needed, but after some
additions it
> > > > > started producing the "control reaches end of function that
requires a
> > > > > return value" or whatever that warning is, and the only way
to eliminate
> > > > > it was with the extra return 0.)
> > > > >
> > > > > If adding the break shuts up the warning, then I guess ACK, but
it would
> > > > > probably be better if 1) gcc fixed their incorrect warning, or
2) we
> > > > > switched the entire function to use the less-compact "ret =
blah;
> > > > > break;" style instead of returning directly, so there
wasn't a single
> > > > > stray break sitting in the middle.
> > > > >
> > > >
> > > > I would say NACK since 1) is the correct option (at least for now),
> > > > there is no reason for adding more lines of code that don't make
sense
> > > > just because of a compiler version that was not released yet, or
does
> > > > not even have a release plan yet.
> > >
> > > GCC 7 *is* released - and has even had a bug fix release too, so ignoring
> > > this is not an option. In any case, as Eric mentions this is a genuine
> > > bug in our code since we can fall out of the inner switch if the input
> > > variable contains a value that doesn't map to an named enum value.
> > >
> >
> > Where did you get the package/tarball? I don't see anything in the
> > release page [1]. On the other hand, when I checked it yesterday, I
> > looked and the development timeline [2] and I thought it's 2016
> > apparently because when I see the dates now it makes sense that the
> > release should be around the corner. Anyway, even if they did not
> > update the release page, on snapshot ftp [3] there is not even a release
> > candidate.
>
> I didn't use any tarball - just what's in Fedora which is
> gcc-7.0.1-0.9.fc26.x86_64
>
> Fedora dist-git says the tarball is gcc-7.0.1-20170219.tar.bz2
>
> Odd that its not on the download page though as that's a clearly a
> release version number, not a git snapshot or pre-release version.
>
The timestamp indicates it's just a snapshot. GCC doesn't do x.0
releases since gcc-5, I believe, so unless it's 7.1 it's not an official
release.
> > I remember others not being happy when we were doing workarounds for
> > packages that downstream distros just decided to package out of VCS or
> > snapshots. I don't feel it's right either and I thought you're on
that
> > side as well. Anyway, if it really was released, I am OK with this
> > going in.
>
> Regardless of whether its a release or pre-release, this is a clear
> bug in the code that needs fixing - its just not a workaround for a
> compiler. As such I've pushed this series.
>
As Laine said as well, there is no bug in the code. All the codepaths
in that switch case end with return.
No they don't. Nothing in C guarantees that dev->type only contains values
that are declared in the enum, particularly since in the struct it is
actually an int. We are pretty careful to only store valid values but
there's no guarantee we did it correctly, so compiler is right to warn
about this.
As an example, the following is entirely valid from the C POV
dev->type = 10000000;
switch ((virDomainDeviceType) dev->type) {
and clearly 10000000 is not handled by any switch case. This is
no different to us screwing up elsewhere in our code and accidentally
setting an invalid enum value.
And it *is* clearly a bug in
the
compiler as there are other cases (e.g. VIR_DOMAIN_CONTROLLER_TYPE_SCSI)
that behave the same, there is no break after it and the compiler is
clearly OK with all the other ones.
The bug is in the warning it does *not* produce - it should be warning
for that other case too because it suffers the same bug.
Regards,
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://entangle-photo.org -o- http://search.cpan.org/~danberr/ :|
3 p.m.
New subject: [libvirt] [PATCH 3/4] qemu: add missing break in qemuDomainDeviceCalculatePCIConnectFlags
On 02/22/2017 01:44 PM, Laine Stump wrote:
On 02/22/2017 12:52 PM, Daniel P. Berrange wrote:
> One of the conditions in qemuDomainDeviceCalculatePCIConnectFlags
> was missing a break that could result it in falling through to
> an incorrect codepath.
Actually that's not true. Every codepath of the preceding case ends with
a "return blah".
Every explicit codepath. But there are implicit codepaths - the default
case. Such paths are unreachable, unless dev->type can ever hold a
value that doesn't map to virDomainDeviceType (since we are relying the
the compiler to fail compilation if we expand the enum but forget to
expand the switch, by intentionally omitting our default case).
This is true for the entire function - every case of
every switch in the function ends with "return blah". The entire purpose
of the function is to determine the flags value, and there are no
resources that need cleaning up before returning, so as soon as the
value is determined, it immediately returns.
Gcc is correct - we can fall through to the outer case
VIR_DOMAIN_DEVICE_FS: label from the outer case
VIR_DOMAIN_DEVICE_CONTROLLER: label, if the inner switch
((virDomainControllerType) cont->type) lands on the (implicit) default:
branch because of storing a non-enum value.
Such a fallthrough is never going to happen in real life, but adding the
break shuts up the compiler, for the same reason that the function ends
with:
/* We can never get here, because all cases are covered in the
* switch, and they all return, but the compiler will still
* complain "control reaches end of non-void function" unless
* we add the following return.
*/
return 0;
If we weren't opposed to abort() in library code, we could change both
the 'break;' and the 'return 0;' into 'abort();', to make it
obvious
that yes, we really do believe that the implicit default case will never
be taken.
I suppose it could be rewritten to change all of those into "ret = blah;
break;", then "return ret;" at the end, but it seemed safer to return
immediately than to trust that no new code would be added later in the
function (and also it's more compact)
I wonder if this is just a more extreme case of the logic in whatever
check necessitated that I add an extra "return 0" at the very end of the
function. (that happens even in gcc 6.x; at an earlier point when the
function was simpler, that wasn't needed, but after some additions it
started producing the "control reaches end of function that requires a
return value" or whatever that warning is, and the only way to eliminate
it was with the extra return 0.)
Yes, it's the same issue.
If adding the break shuts up the warning, then I guess ACK, but it would
probably be better if 1) gcc fixed their incorrect warning, or 2) we
switched the entire function to use the less-compact "ret = blah;
break;" style instead of returning directly, so there wasn't a single
stray break sitting in the middle.
I don't think the warning is incorrect.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
3:39 a.m.
New subject: [libvirt] [PATCH 3/4] qemu: add missing break in qemuDomainDeviceCalculatePCIConnectFlags
On Wed, Feb 22, 2017 at 03:00:41PM -0600, Eric Blake wrote:
On 02/22/2017 01:44 PM, Laine Stump wrote:
> On 02/22/2017 12:52 PM, Daniel P. Berrange wrote:
>> One of the conditions in qemuDomainDeviceCalculatePCIConnectFlags
>> was missing a break that could result it in falling through to
>> an incorrect codepath.
>
> Actually that's not true. Every codepath of the preceding case ends with
> a "return blah".
Every explicit codepath. But there are implicit codepaths - the default
case. Such paths are unreachable, unless dev->type can ever hold a
value that doesn't map to virDomainDeviceType (since we are relying the
the compiler to fail compilation if we expand the enum but forget to
expand the switch, by intentionally omitting our default case).
Yep and such a scenario can happen, because the compiler can't bounds
check the value when you have code that casts from an int to the enum,
which is exactly what we do when parsing XML.
> This is true for the entire function - every case of
> every switch in the function ends with "return blah". The entire purpose
> of the function is to determine the flags value, and there are no
> resources that need cleaning up before returning, so as soon as the
> value is determined, it immediately returns.
Gcc is correct - we can fall through to the outer case
VIR_DOMAIN_DEVICE_FS: label from the outer case
VIR_DOMAIN_DEVICE_CONTROLLER: label, if the inner switch
((virDomainControllerType) cont->type) lands on the (implicit) default:
branch because of storing a non-enum value.
Such a fallthrough is never going to happen in real life, but adding the
break shuts up the compiler, for the same reason that the function ends
with:
I could happen in real-life if we have a bug in the code which populates
the dev type value since nothing bounds checks the values assigned at
runtime.
> If adding the break shuts up the warning, then I guess ACK, but
it would
> probably be better if 1) gcc fixed their incorrect warning, or 2) we
> switched the entire function to use the less-compact "ret = blah;
> break;" style instead of returning directly, so there wasn't a single
> stray break sitting in the middle.
I don't think the warning is incorrect.
Indeed, and if the nested switch had a default: case which returnd
a value, then the warning would not be triggered.
Regards,
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://entangle-photo.org -o- http://search.cpan.org/~danberr/ :|
2:52 p.m.
New subject: [libvirt] [PATCH 3/4] qemu: add missing break in qemuDomainDeviceCalculatePCIConnectFlags
On 02/22/2017 11:52 AM, Daniel P. Berrange wrote:
One of the conditions in qemuDomainDeviceCalculatePCIConnectFlags
was missing a break that could result it in falling through to
an incorrect codepath.
Signed-off-by: Daniel P. Berrange <berrange(a)redhat.com>
---
src/qemu/qemu_domain_address.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c
index 5b75044..27ca010 100644
--- a/src/qemu/qemu_domain_address.c
+++ b/src/qemu/qemu_domain_address.c
@@ -553,6 +553,7 @@ qemuDomainDeviceCalculatePCIConnectFlags(virDomainDeviceDefPtr dev,
return pciFlags;
}
}
+ break;
Indentation looks weird. I guess its because we have an extra {} scope
for allowing declaration of variables? Would hoisting the declaration
(not initialization) of
case VIR_DOMAIN_DEVICE_CONTROLLER: {
virDomainControllerDefPtr cont = dev->data.controller;
allow us to get rid of the weird {}?
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
3:41 a.m.
New subject: [libvirt] [PATCH 3/4] qemu: add missing break in qemuDomainDeviceCalculatePCIConnectFlags
On Wed, Feb 22, 2017 at 02:52:08PM -0600, Eric Blake wrote:
On 02/22/2017 11:52 AM, Daniel P. Berrange wrote:
> One of the conditions in qemuDomainDeviceCalculatePCIConnectFlags
> was missing a break that could result it in falling through to
> an incorrect codepath.
>
> Signed-off-by: Daniel P. Berrange <berrange(a)redhat.com>
> ---
> src/qemu/qemu_domain_address.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c
> index 5b75044..27ca010 100644
> --- a/src/qemu/qemu_domain_address.c
> +++ b/src/qemu/qemu_domain_address.c
> @@ -553,6 +553,7 @@ qemuDomainDeviceCalculatePCIConnectFlags(virDomainDeviceDefPtr
dev,
> return pciFlags;
> }
> }
> + break;
Indentation looks weird. I guess its because we have an extra {} scope
for allowing declaration of variables? Would hoisting the declaration
(not initialization) of
case VIR_DOMAIN_DEVICE_CONTROLLER: {
virDomainControllerDefPtr cont = dev->data.controller;
allow us to get rid of the weird {}?
Yes, though this style is not that unusual - we've got it in a number
of other places in libvirt already where we want to isolate the
variable declaration, so I think this is ok as is.
Regards,
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://entangle-photo.org -o- http://search.cpan.org/~danberr/ :|
11:52 a.m.
New subject: [libvirt] [PATCH 4/4] Add ATTRIBUTE_FALLTHROUGH for switch cases without break
In GCC 7 there is a new warning triggered when a switch
case has a conditional statement (eg if ... else...) and
some of the code paths fallthrough to the next switch
statement. e.g.
conf/domain_conf.c: In function 'virDomainChrEquals':
conf/domain_conf.c:14926:12: error: this statement may fall through
[-Werror=implicit-fallthrough=]
if (src->targetTypeAttr != tgt->targetTypeAttr)
^
conf/domain_conf.c:14928:5: note: here
case VIR_DOMAIN_CHR_DEVICE_TYPE_CONSOLE:
^~~~
conf/domain_conf.c: In function 'virDomainChrDefFormat':
conf/domain_conf.c:22143:12: error: this statement may fall through
[-Werror=implicit-fallthrough=]
if (def->targetTypeAttr) {
^
conf/domain_conf.c:22151:5: note: here
default:
^~~~~~~
GCC introduced a __attribute__((fallthrough)) to let you
indicate that this is intentionale behaviour rather than
a bug.
Signed-off-by: Daniel P. Berrange <berrange(a)redhat.com>
---
src/conf/domain_conf.c | 7 +++++++
src/conf/network_conf.c | 3 ++-
src/internal.h | 8 ++++++++
src/lxc/lxc_container.c | 2 +-
src/network/bridge_driver.c | 6 ++++++
tools/virsh-edit.c | 2 +-
6 files changed, 25 insertions(+), 3 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index f718b9a..17995f7 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -14925,7 +14925,12 @@ virDomainChrEquals(virDomainChrDefPtr src,
case VIR_DOMAIN_CHR_DEVICE_TYPE_SERIAL:
if (src->targetTypeAttr != tgt->targetTypeAttr)
return false;
+
+ ATTRIBUTE_FALLTHROUGH;
+
case VIR_DOMAIN_CHR_DEVICE_TYPE_CONSOLE:
+ ATTRIBUTE_FALLTHROUGH;
+
case VIR_DOMAIN_CHR_DEVICE_TYPE_PARALLEL:
return src->target.port == tgt->target.port;
break;
@@ -22148,6 +22153,8 @@ virDomainChrDefFormat(virBufferPtr buf,
def->target.port);
break;
}
+ ATTRIBUTE_FALLTHROUGH;
+
default:
virBufferAsprintf(buf, "<target port='%d'/>\n",
def->target.port);
diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c
index 0e20dac..48e0001 100644
--- a/src/conf/network_conf.c
+++ b/src/conf/network_conf.c
@@ -2442,7 +2442,8 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt)
def->name);
goto error;
}
- /* fall through to next case */
+ ATTRIBUTE_FALLTHROUGH;
+
case VIR_NETWORK_FORWARD_BRIDGE:
if (def->delay || stp) {
virReportError(VIR_ERR_XML_ERROR,
diff --git a/src/internal.h b/src/internal.h
index 334659d..74a43fa 100644
--- a/src/internal.h
+++ b/src/internal.h
@@ -218,6 +218,10 @@
# endif
# endif
+# ifndef ATTRIBUTE_FALLTHROUGH
+# define ATTRIBUTE_FALLTHROUGH __attribute__((fallthrough))
+# endif
+
# else
# ifndef ATTRIBUTE_UNUSED
# define ATTRIBUTE_UNUSED
@@ -228,6 +232,10 @@
# ifndef ATTRIBUTE_RETURN_CHECK
# define ATTRIBUTE_RETURN_CHECK
# endif
+#
+# ifndef ATTRIBUTE_FALLTHROUGH
+# define ATTRIBUTE_FALLTHROUGH do {} while(0)
+# endif
# endif /* __GNUC__ */
diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c
index 601b9b0..99bd7e9 100644
--- a/src/lxc/lxc_container.c
+++ b/src/lxc/lxc_container.c
@@ -2042,7 +2042,7 @@ static int lxcContainerDropCapabilities(virDomainDefPtr def,
default: /* User specified capabilities to drop */
toDrop = (state == VIR_TRISTATE_SWITCH_OFF);
}
- /* Fallthrough */
+ ATTRIBUTE_FALLTHROUGH;
case VIR_DOMAIN_CAPABILITIES_POLICY_ALLOW:
if (policy == VIR_DOMAIN_CAPABILITIES_POLICY_ALLOW)
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index 06759c6..c5ec282 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -2715,6 +2715,8 @@ networkStartNetwork(virNetworkDriverStatePtr driver,
* VIR_NETWORK_FORWARD_BRIDGE with no bridge device defined
* (since that is macvtap bridge mode).
*/
+ ATTRIBUTE_FALLTHROUGH;
+
case VIR_NETWORK_FORWARD_PRIVATE:
case VIR_NETWORK_FORWARD_VEPA:
case VIR_NETWORK_FORWARD_PASSTHROUGH:
@@ -2792,6 +2794,8 @@ networkShutdownNetwork(virNetworkDriverStatePtr driver,
* VIR_NETWORK_FORWARD_BRIDGE with no bridge device defined
* (since that is macvtap bridge mode).
*/
+ ATTRIBUTE_FALLTHROUGH;
+
case VIR_NETWORK_FORWARD_PRIVATE:
case VIR_NETWORK_FORWARD_VEPA:
case VIR_NETWORK_FORWARD_PASSTHROUGH:
@@ -4974,6 +4978,8 @@ networkGetNetworkAddress(const char *netname, char **netaddr)
* fall through if netdef->bridge wasn't set, since that is
* macvtap bridge mode network.
*/
+ ATTRIBUTE_FALLTHROUGH;
+
case VIR_NETWORK_FORWARD_PRIVATE:
case VIR_NETWORK_FORWARD_VEPA:
case VIR_NETWORK_FORWARD_PASSTHROUGH:
diff --git a/tools/virsh-edit.c b/tools/virsh-edit.c
index 16d6705..92a00b7 100644
--- a/tools/virsh-edit.c
+++ b/tools/virsh-edit.c
@@ -140,7 +140,7 @@ do {
goto redefine;
break;
}
- /* fall-through */
+ ATTRIBUTE_FALLTHROUGH;
#endif
default:
--
2.9.3
11:57 a.m.
New subject: [libvirt] [PATCH 4/4] Add ATTRIBUTE_FALLTHROUGH for switch cases without break
On Wed, Feb 22, 2017 at 05:52:05PM +0000, Daniel P. Berrange wrote:
In GCC 7 there is a new warning triggered when a switch
case has a conditional statement (eg if ... else...) and
some of the code paths fallthrough to the next switch
statement. e.g.
conf/domain_conf.c: In function 'virDomainChrEquals':
conf/domain_conf.c:14926:12: error: this statement may fall through
[-Werror=implicit-fallthrough=]
if (src->targetTypeAttr != tgt->targetTypeAttr)
^
conf/domain_conf.c:14928:5: note: here
case VIR_DOMAIN_CHR_DEVICE_TYPE_CONSOLE:
^~~~
conf/domain_conf.c: In function 'virDomainChrDefFormat':
conf/domain_conf.c:22143:12: error: this statement may fall through
[-Werror=implicit-fallthrough=]
if (def->targetTypeAttr) {
^
conf/domain_conf.c:22151:5: note: here
default:
^~~~~~~
GCC introduced a __attribute__((fallthrough)) to let you
indicate that this is intentionale behaviour rather than
a bug.
BTW, CLang has apparently had an -Wimplicit-fallthrough
warning flag for a while, and also seems to have the same
__attribute__((fallthrough)), but I've been unable to get
CLang to trigger such warnings, and hence did not enable
__attribute__((fallthrough)) on CLang. If someone can
figure out how to reproduce the warnings on clang we
could extend internal.h to surpress them for whichever
clang version introduced the warnings. As is, the
ATTRIBUTE_FALLTHROUGH turns into a no-op for CLang unless
it claims GCC 7.0 compatibility.
Regards,
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://entangle-photo.org -o- http://search.cpan.org/~danberr/ :|
11:07 a.m.
New subject: [libvirt] [PATCH 4/4] Add ATTRIBUTE_FALLTHROUGH for switch cases without break
Daniel P. Berrange wrote:
On Wed, Feb 22, 2017 at 05:52:05PM +0000, Daniel P. Berrange wrote:
> In GCC 7 there is a new warning triggered when a switch
> case has a conditional statement (eg if ... else...) and
> some of the code paths fallthrough to the next switch
> statement. e.g.
>
> conf/domain_conf.c: In function 'virDomainChrEquals':
> conf/domain_conf.c:14926:12: error: this statement may fall through
[-Werror=implicit-fallthrough=]
> if (src->targetTypeAttr != tgt->targetTypeAttr)
> ^
> conf/domain_conf.c:14928:5: note: here
> case VIR_DOMAIN_CHR_DEVICE_TYPE_CONSOLE:
> ^~~~
> conf/domain_conf.c: In function 'virDomainChrDefFormat':
> conf/domain_conf.c:22143:12: error: this statement may fall through
[-Werror=implicit-fallthrough=]
> if (def->targetTypeAttr) {
> ^
> conf/domain_conf.c:22151:5: note: here
> default:
> ^~~~~~~
>
> GCC introduced a __attribute__((fallthrough)) to let you
> indicate that this is intentionale behaviour rather than
> a bug.
BTW, CLang has apparently had an -Wimplicit-fallthrough
warning flag for a while, and also seems to have the same
__attribute__((fallthrough)), but I've been unable to get
CLang to trigger such warnings, and hence did not enable
__attribute__((fallthrough)) on CLang. If someone can
figure out how to reproduce the warnings on clang we
could extend internal.h to surpress them for whichever
clang version introduced the warnings. As is, the
ATTRIBUTE_FALLTHROUGH turns into a no-op for CLang unless
it claims GCC 7.0 compatibility.
It *looks* like -Wimplicit-fallthrough in clang is only effective for
C++11:
http://clang-developers.42468.n3.nabble.com/should-Wimplicit-fallthrough-...
It mentions this commit:
http://llvm.org/viewvc/llvm-project?revision=167655&view=revision
And it seems it wasn't changed since than.
Roman Bogorodskiy
11:13 a.m.
New subject: [libvirt] [PATCH 4/4] Add ATTRIBUTE_FALLTHROUGH for switch cases without break
On Thu, Feb 23, 2017 at 09:07:59PM +0400, Roman Bogorodskiy wrote:
Daniel P. Berrange wrote:
> On Wed, Feb 22, 2017 at 05:52:05PM +0000, Daniel P. Berrange wrote:
> > In GCC 7 there is a new warning triggered when a switch
> > case has a conditional statement (eg if ... else...) and
> > some of the code paths fallthrough to the next switch
> > statement. e.g.
> >
> > conf/domain_conf.c: In function 'virDomainChrEquals':
> > conf/domain_conf.c:14926:12: error: this statement may fall through
[-Werror=implicit-fallthrough=]
> > if (src->targetTypeAttr != tgt->targetTypeAttr)
> > ^
> > conf/domain_conf.c:14928:5: note: here
> > case VIR_DOMAIN_CHR_DEVICE_TYPE_CONSOLE:
> > ^~~~
> > conf/domain_conf.c: In function 'virDomainChrDefFormat':
> > conf/domain_conf.c:22143:12: error: this statement may fall through
[-Werror=implicit-fallthrough=]
> > if (def->targetTypeAttr) {
> > ^
> > conf/domain_conf.c:22151:5: note: here
> > default:
> > ^~~~~~~
> >
> > GCC introduced a __attribute__((fallthrough)) to let you
> > indicate that this is intentionale behaviour rather than
> > a bug.
>
>
> BTW, CLang has apparently had an -Wimplicit-fallthrough
> warning flag for a while, and also seems to have the same
> __attribute__((fallthrough)), but I've been unable to get
> CLang to trigger such warnings, and hence did not enable
> __attribute__((fallthrough)) on CLang. If someone can
> figure out how to reproduce the warnings on clang we
> could extend internal.h to surpress them for whichever
> clang version introduced the warnings. As is, the
> ATTRIBUTE_FALLTHROUGH turns into a no-op for CLang unless
> it claims GCC 7.0 compatibility.
It *looks* like -Wimplicit-fallthrough in clang is only effective for
C++11:
http://clang-developers.42468.n3.nabble.com/should-Wimplicit-fallthrough-...
It mentions this commit:
http://llvm.org/viewvc/llvm-project?revision=167655&view=revision
And it seems it wasn't changed since than.
Ah I see. So they disabled it for C, because their [[clang::fallthrough]]
magic annotation would not work under C. Hopefully they'll enable it one
day with the more normal __attribute__((fallthrough)) syntax for C.
Regards,
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://entangle-photo.org -o- http://search.cpan.org/~danberr/ :|
1:46 p.m.
New subject: [libvirt] [PATCH 4/4] Add ATTRIBUTE_FALLTHROUGH for switch cases without break
On 02/22/2017 12:52 PM, Daniel P. Berrange wrote:
In GCC 7 there is a new warning triggered when a switch
case has a conditional statement (eg if ... else...) and
some of the code paths fallthrough to the next switch
statement. e.g.
conf/domain_conf.c: In function 'virDomainChrEquals':
conf/domain_conf.c:14926:12: error: this statement may fall through
[-Werror=implicit-fallthrough=]
if (src->targetTypeAttr != tgt->targetTypeAttr)
^
conf/domain_conf.c:14928:5: note: here
case VIR_DOMAIN_CHR_DEVICE_TYPE_CONSOLE:
^~~~
conf/domain_conf.c: In function 'virDomainChrDefFormat':
conf/domain_conf.c:22143:12: error: this statement may fall through
[-Werror=implicit-fallthrough=]
if (def->targetTypeAttr) {
^
conf/domain_conf.c:22151:5: note: here
default:
^~~~~~~
GCC introduced a __attribute__((fallthrough)) to let you
indicate that this is intentionale behaviour rather than
a bug.
ACK.
2:49 p.m.
New subject: [libvirt] [PATCH 4/4] Add ATTRIBUTE_FALLTHROUGH for switch cases without break
On 02/22/2017 11:52 AM, Daniel P. Berrange wrote:
In GCC 7 there is a new warning triggered when a switch
case has a conditional statement (eg if ... else...) and
some of the code paths fallthrough to the next switch
statement. e.g.
conf/domain_conf.c: In function 'virDomainChrEquals':
conf/domain_conf.c:14926:12: error: this statement may fall through
[-Werror=implicit-fallthrough=]
if (src->targetTypeAttr != tgt->targetTypeAttr)
^
+++ b/src/conf/domain_conf.c
@@ -14925,7 +14925,12 @@ virDomainChrEquals(virDomainChrDefPtr src,
case VIR_DOMAIN_CHR_DEVICE_TYPE_SERIAL:
if (src->targetTypeAttr != tgt->targetTypeAttr)
return false;
+
+ ATTRIBUTE_FALLTHROUGH;
+
I understand this one...
case VIR_DOMAIN_CHR_DEVICE_TYPE_CONSOLE:
+ ATTRIBUTE_FALLTHROUGH;
+
case VIR_DOMAIN_CHR_DEVICE_TYPE_PARALLEL:
...but was this one necessary, or is gcc smart enough to know that two
consecutive labels never needs an explicit fallthrough?
Also, is it sufficient to spell it:
/* fall through */
so that Coverity can also recognize it? Or does gcc not recognize the
magic comment?
+++ b/src/internal.h
@@ -218,6 +218,10 @@
# endif
# endif
+# ifndef ATTRIBUTE_FALLTHROUGH
+# define ATTRIBUTE_FALLTHROUGH __attribute__((fallthrough))
+# endif
If the magic /* fall through */ comment is sufficient, then we don't
need this macro.
+++ b/src/lxc/lxc_container.c
@@ -2042,7 +2042,7 @@ static int lxcContainerDropCapabilities(virDomainDefPtr def,
default: /* User specified capabilities to drop */
toDrop = (state == VIR_TRISTATE_SWITCH_OFF);
}
- /* Fallthrough */
+ ATTRIBUTE_FALLTHROUGH;
Hmm - this argues at least one comment spelling that gcc does not recognize.
+++ b/tools/virsh-edit.c
@@ -140,7 +140,7 @@ do {
goto redefine;
break;
}
- /* fall-through */
+ ATTRIBUTE_FALLTHROUGH;
#endif
and another.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
3:44 a.m.
New subject: [libvirt] [PATCH 4/4] Add ATTRIBUTE_FALLTHROUGH for switch cases without break
On Wed, Feb 22, 2017 at 02:49:12PM -0600, Eric Blake wrote:
On 02/22/2017 11:52 AM, Daniel P. Berrange wrote:
> In GCC 7 there is a new warning triggered when a switch
> case has a conditional statement (eg if ... else...) and
> some of the code paths fallthrough to the next switch
> statement. e.g.
>
> conf/domain_conf.c: In function 'virDomainChrEquals':
> conf/domain_conf.c:14926:12: error: this statement may fall through
[-Werror=implicit-fallthrough=]
> if (src->targetTypeAttr != tgt->targetTypeAttr)
> ^
> +++ b/src/conf/domain_conf.c
> @@ -14925,7 +14925,12 @@ virDomainChrEquals(virDomainChrDefPtr src,
> case VIR_DOMAIN_CHR_DEVICE_TYPE_SERIAL:
> if (src->targetTypeAttr != tgt->targetTypeAttr)
> return false;
> +
> + ATTRIBUTE_FALLTHROUGH;
> +
I understand this one...
> case VIR_DOMAIN_CHR_DEVICE_TYPE_CONSOLE:
> + ATTRIBUTE_FALLTHROUGH;
> +
> case VIR_DOMAIN_CHR_DEVICE_TYPE_PARALLEL:
...but was this one necessary, or is gcc smart enough to know that two
consecutive labels never needs an explicit fallthrough?
Yeah, that second one was bogus - i added it by mistake.
Also, is it sufficient to spell it:
/* fall through */
so that Coverity can also recognize it? Or does gcc not recognize the
magic comment?
IIUC, it only likes an all lowercase 'fallthrough' comment, but I'm
not inclined to use that. CLang doesn't parse comments at all and
relies on the attribute annotation.
Given that both GCC & CLang now support the same attribute name for
this, I would expect Coverity will learn about it too in the not too
distant future.
Regards,
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://entangle-photo.org -o- http://search.cpan.org/~danberr/ :|
2832
days inactive
2833
days old
26 comments
7 participants
participants (7)
-
Andrea Bolognani -
Daniel P. Berrange -
Eric Blake -
Laine Stump -
Martin Kletzander -
Michal Privoznik -
Roman Bogorodskiy