[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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

Attachment: signature.asc
Description: Digital signature


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]