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

Thu Feb 23 12:15:50 UTC 2017

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 at redhat.com
>https://www.redhat.com/mailman/listinfo/libvir-list
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20170223/682b37a4/attachment-0001.sig>