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

[Martin Kletzander]

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.

>
>>
>> Signed-off-by: Daniel P. Berrange <berrange at redhat.com>
>> ---
>>   src/qemu/qemu_domain_address.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c
>> index 5b75044..27ca010 100644
>> --- a/src/qemu/qemu_domain_address.c
>> +++ b/src/qemu/qemu_domain_address.c
>> @@ -553,6 +553,7 @@ qemuDomainDeviceCalculatePCIConnectFlags(virDomainDeviceDefPtr dev,
>>               return pciFlags;
>>           }
>>       }
>> +        break;
>>
>>       case VIR_DOMAIN_DEVICE_FS:
>>           /* the only type of filesystem so far is virtio-9p-pci */
>
>
>--
>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/20170222/e05bf66c/attachment-0001.sig>