Re: [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
Attachments:
- signature.asc (application/pgp-signature — 604 bytes)