[libvirt] [PATCH 3/4] qemu: add missing break in qemuDomainDeviceCalculatePCIConnectFlags

[Daniel P. Berrange]

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/ :|