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

[Daniel P. Berrange]

On Wed, Feb 22, 2017 at 03:00:41PM -0600, Eric Blake wrote:
> 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).

Yep and such a scenario can happen, because the compiler can't bounds
check the value when you have code that casts from an int to the enum,
which is exactly what we do when parsing XML. 

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

I could happen in real-life if we have a bug in the code which populates
the dev type value since nothing bounds checks the values assigned at
runtime.


> > 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.

Indeed, and if the nested switch had a default: case which returnd
a value, then the warning would not be triggered.


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