Hi Michal,
On 01/08/2022 09:23, Michal Prívozník wrote:
On 7/29/22 17:50, Oleksandr Tyshchenko wrote:
From: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
Xen toolstack has gained basic Virtio support recently which becides
adding various virtio related stuff introduces new disk backend type
LIBXL_DISK_BACKEND_STANDALONE [1].
Unfortunately, this caused a regression in libvirt build with Xen support
enabled, reported by the osstest today [2]:
CC libxl/libvirt_driver_libxl_impl_la-xen_xl.lo
../../src/libxl/xen_xl.c: In function 'xenParseXLDisk':
../../src/libxl/xen_xl.c:779:17: error: enumeration value 'LIBXL_DISK_BACKEND_STANDALONE'
not handled in switch [-Werror=switch-enum]
switch (libxldisk->backend) {
^~~~~~
cc1: all warnings being treated as errors
The interesting fact is that switch already has a default branch (which ought
to cover such new addition), but the error is triggered as -Wswitch-enum
gives a warning about an omitted enumeration code even if there is a default
label.
This is expected and in fact working correctly. We want compiler to warn
us about enum members that are not handled in a switch() statement.
For us this is treated as an error. Is it intended?
If it is, then I think this will be a problem for Xen because it means
we will always need to fix libvirt before accepting a patch in Xen (see
more below).
The
'default' case exists in some places because we suspect the value might
not have been validated before. For instance:
libxl_disk_backend x = atoi(argv[1]); /* or parse something from XML */
switch(x) {
case LIBXL_DISK_BACKEND_UNKNOWN:
case LIBXL_DISK_BACKEND_PHY:
case LIBXL_DISK_BACKEND_TAP:
case LIBXL_DISK_BACKEND_QDISK:
// Neither of these might be exectuted ..
default:
// .. in which case this will.
}
But we are not very consistent in putting 'default' case, sadly.
Also there is a similar issue in libxlUpdateDiskDef() which I have reproduced
after fixing the first one, but it that case the corresponding switch doesn't
have a default branch.
Fix both issues by inserting required enumeration item to make the compiler
happy and adding ifdef guard to be able to build against old Xen libraries
as well (without LIBXL_HAVE_DEVICE_DISK_SPECIFICATION). Also add a default
branch to switch in libxlUpdateDiskDef().
Please note, that current patch doesn't implement the proper handling of
LIBXL_DISK_BACKEND_STANDALONE and friends, it is just intended to fix
the regression immediately to unblock the osstest. Also it worth mentioning
that current patch won't solve the possible additions in the future.
[1] https://lore.kernel.org/xen-devel/20220716163745.28712-1-olekstysh@xxxxxxxxx/
[2] https://lore.kernel.org/xen-devel/E1oHEQO-0008GA-Uo@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx/
Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
---
Cc: Julien Grall <julien@xxxxxxx>
Cc: Anthony PERARD <anthony.perard@xxxxxxxxxx>
Cc: Michal Privoznik <mprivozn@xxxxxxxxxx>
Please note, the patch is tested on:
https://xenbits.xen.org/gitweb/?p=libvirt.git;a=shortlog;h=refs/heads/xen-tested-master
but should work on the master as well (as the same code is present here).
---
src/libxl/libxl_conf.c | 4 ++++
src/libxl/xen_xl.c | 3 +++
2 files changed, 7 insertions(+)
Ah, I couldn't find the commit in master, and it's simply because it's
not there yet. It's in staging:
https://xenbits.xen.org/gitweb/?p=xen.git;a=commit;f=tools/libs/light/libxl_types.idl;h=66dd1c62b2a3c707bd5c55750d10a8223fbd577f
The patch looks correct. Do you have any estimate when it can be merged
into master? I'm not sure what our, libvirt, rules about xen staging
are, but for qemu we require master (even unreleased yet).
The patches usually land in master after our test suite has completed.
One of the test is to confirm that libvirt is still working. Therefore,
the Xen patch will not be part of master until the patch in libvirt is
added.
Cheers,
--
Julien Grall
