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