[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 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 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 redhat com
https://www.redhat.com/mailman/listinfo/libvir-list

Attachment: signature.asc
Description: Digital signature


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