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