On Tue, Feb 13, 2018 at 04:57:06PM +0000, Daniel P. Berrangé wrote:
GCC 8 became more fussy/clear about detecting switch
fallthroughs. First it doesn't like it if you have
a fallthrough attribute that is not before a case
statement. e.g.
FOO:
BAR:
WIZZ:
ATTRIBUTE_FALLTHROUGH;
Is unacceptable as there's no final case statement,
so while FOO & BAR are falling through, WIZZ is
not falling through. IOW, GCC wants us to write
FOO:
BAR:
ATTRIBUTE_FALLTHROUGH;
WIZZ:
Second, it will report risk of fallthrough even if you
have a case statement for every single enum value, but
only if the switch is nested inside another switch and
the outer case statement has no final break. This is
is in fact valid because despite the fact that we have
cast from "int" to the enum typedef, nothing guarantees
that the variable we're switching on only contains values
that have corresponding switch labels. e.g.
int domstate = 87539319;
switch ((virDomainState)domstate) {
...
}
will not match enum value, but also not raise any kind
of compiler warning. So it is right to complain about
risk of fallthrough if no default: is present.
Arrrgh, and indeed we've really got such brokenness in our
code.
virDomainControllerDefNew() sets model == -1, when
qemuDomainDeviceCalculatePCIConnectFlags() is run, for the
auto-added USB controller we fallthrough to the case
statement that handles IDE. Fixing the broken fallthrough
in qemuDomainDeviceCalculatePCIConnectFlags() in turn
breaks many of the qemu test cases :-( What a horrible
mess.
This is a really strong sign that we should *never* rely
on listing all enum values in a switch() and must always
have a default: too.
Anyway NACK to this patch for now until I can figure out
how to unbreak the existing flaws.
Signed-off-by: Daniel P. Berrangé <berrange(a)redhat.com>
---
src/qemu/qemu_domain.c | 14 +++++++-------
src/qemu/qemu_domain_address.c | 11 +++++++++++
2 files changed, 18 insertions(+), 7 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index f6e28447d..e262588c3 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -175,9 +175,9 @@ qemuDomainAsyncJobPhaseToString(qemuDomainAsyncJob job,
case QEMU_ASYNC_JOB_NONE:
case QEMU_ASYNC_JOB_LAST:
ATTRIBUTE_FALLTHROUGH;
+ default:
+ return "none";
}
-
- return "none";
}
int
@@ -199,12 +199,12 @@ qemuDomainAsyncJobPhaseFromString(qemuDomainAsyncJob job,
case QEMU_ASYNC_JOB_NONE:
case QEMU_ASYNC_JOB_LAST:
ATTRIBUTE_FALLTHROUGH;
+ default:
+ if (STREQ(phase, "none"))
+ return 0;
+ else
+ return -1;
}
-
- if (STREQ(phase, "none"))
- return 0;
- else
- return -1;
}
diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c
index e28119e4b..42c7c0b12 100644
--- a/src/qemu/qemu_domain_address.c
+++ b/src/qemu/qemu_domain_address.c
@@ -532,6 +532,7 @@ qemuDomainDeviceCalculatePCIConnectFlags(virDomainDeviceDefPtr dev,
case VIR_DOMAIN_CONTROLLER_MODEL_USB_QUSB2: /* xen only */
case VIR_DOMAIN_CONTROLLER_MODEL_USB_NONE:
case VIR_DOMAIN_CONTROLLER_MODEL_USB_LAST:
+ default:
return 0;
}
@@ -553,6 +554,7 @@ qemuDomainDeviceCalculatePCIConnectFlags(virDomainDeviceDefPtr dev,
return pciFlags;
case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LAST:
+ default:
return 0;
}
@@ -562,6 +564,7 @@ qemuDomainDeviceCalculatePCIConnectFlags(virDomainDeviceDefPtr dev,
case VIR_DOMAIN_CONTROLLER_TYPE_FDC:
case VIR_DOMAIN_CONTROLLER_TYPE_CCID:
case VIR_DOMAIN_CONTROLLER_TYPE_LAST:
+ default:
/* should be 0 */
return pciFlags;
}
@@ -605,6 +608,7 @@ qemuDomainDeviceCalculatePCIConnectFlags(virDomainDeviceDefPtr dev,
case VIR_DOMAIN_SOUND_MODEL_PCSPK:
case VIR_DOMAIN_SOUND_MODEL_USB:
case VIR_DOMAIN_SOUND_MODEL_LAST:
+ default:
return 0;
}
@@ -622,6 +626,7 @@ qemuDomainDeviceCalculatePCIConnectFlags(virDomainDeviceDefPtr dev,
case VIR_DOMAIN_DISK_BUS_SATA:
case VIR_DOMAIN_DISK_BUS_SD:
case VIR_DOMAIN_DISK_BUS_LAST:
+ default:
return 0;
}
@@ -734,6 +739,7 @@ qemuDomainDeviceCalculatePCIConnectFlags(virDomainDeviceDefPtr dev,
case VIR_DOMAIN_MEMBALLOON_MODEL_XEN:
case VIR_DOMAIN_MEMBALLOON_MODEL_NONE:
case VIR_DOMAIN_MEMBALLOON_MODEL_LAST:
+ default:
return 0;
}
@@ -743,6 +749,7 @@ qemuDomainDeviceCalculatePCIConnectFlags(virDomainDeviceDefPtr dev,
return virtioFlags;
case VIR_DOMAIN_RNG_MODEL_LAST:
+ default:
return 0;
}
@@ -755,6 +762,7 @@ qemuDomainDeviceCalculatePCIConnectFlags(virDomainDeviceDefPtr dev,
case VIR_DOMAIN_WATCHDOG_MODEL_IB700:
case VIR_DOMAIN_WATCHDOG_MODEL_DIAG288:
case VIR_DOMAIN_WATCHDOG_MODEL_LAST:
+ default:
return 0;
}
@@ -775,6 +783,7 @@ qemuDomainDeviceCalculatePCIConnectFlags(virDomainDeviceDefPtr dev,
case VIR_DOMAIN_VIDEO_TYPE_DEFAULT:
case VIR_DOMAIN_VIDEO_TYPE_GOP:
case VIR_DOMAIN_VIDEO_TYPE_LAST:
+ default:
return 0;
}
@@ -791,6 +800,7 @@ qemuDomainDeviceCalculatePCIConnectFlags(virDomainDeviceDefPtr dev,
case VIR_DOMAIN_INPUT_BUS_XEN:
case VIR_DOMAIN_INPUT_BUS_PARALLELS:
case VIR_DOMAIN_INPUT_BUS_LAST:
+ default:
return 0;
}
@@ -806,6 +816,7 @@ qemuDomainDeviceCalculatePCIConnectFlags(virDomainDeviceDefPtr dev,
case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_SCLP:
case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_NONE:
case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_LAST:
+ default:
return 0;
}
--
2.16.1
Regards,
Daniel
--
|:
https://berrange.com -o-
https://www.flickr.com/photos/dberrange :|
|:
https://libvirt.org -o-
https://fstop138.berrange.com :|
|:
https://entangle-photo.org -o-
https://www.instagram.com/dberrange :|