[libvirt PATCH] libxl: Fix build with recent Xen that introduces new disk backend type

Mon Aug 1 11:33:16 UTC 2022

On 01.08.22 13:08, Michal Prívozník wrote:

Hello Michal, Julien

> On 8/1/22 10:51, Julien Grall wrote:
>> 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 at epam.com>
>>>>
>>>> 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?
> -Werror shouldn't be enabled when building a package, exactly for this
> reason. Header files change and we might get a warning or two when
> building a RPM. However, we definitely want to treat warnings as errors
> when developing libvirt, i.e. building libvirt from a git repo. That's
> why we get -Werror enabled in our CI too.
>
>> 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).
> So we have a chicken egg problem. Xen needs libvirt to compile without
> any warning to merge a patch and libvirt wants hypervisors to have the
> patch merged first. Well, I think in this case we can make an
> "exception". Our demand comes from quite a few cases where we burned
> ourselves by merging our portion of a feature before it was merged into
> QEMU. And according to Murphy's law, QEMU interface was changed
> rendering our patches (now commits) useless. But I believe this is not
> the case with xen staging, is it?

I also believe so.

>
> BTW: every other package that does switch() over libxl_disk_backend enum
> will need this fix.
>
>>>   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://urldefense.com/v3/__https://lore.kernel.org/xen-devel/20220716163745.28712-1-olekstysh@gmail.com/__;!!GF_29dbcQIUBPA!2GtOGVnLefUlZqkD4BAl9lVy82Avr_zPfztssIJ0qIBO8m8GTnSS2n2U6MpS0I2MhBxaE0EA9-GqbnlB-bQ9SdxTbA$ [lore[.]kernel[.]org]
>>>>
>>>> [2]
>>>> https://urldefense.com/v3/__https://lore.kernel.org/xen-devel/E1oHEQO-0008GA-Uo@osstest.test-lab.xenproject.org/__;!!GF_29dbcQIUBPA!2GtOGVnLefUlZqkD4BAl9lVy82Avr_zPfztssIJ0qIBO8m8GTnSS2n2U6MpS0I2MhBxaE0EA9-GqbnlB-bTJQY0vsw$ [lore[.]kernel[.]org]
>>>>
>>>>
>>>> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko at epam.com>
>>>> ---
>>>> Cc: Julien Grall <julien at xen.org>
>>>> Cc: Anthony PERARD <anthony.perard at citrix.com>
>>>> Cc: Michal Privoznik <mprivozn at redhat.com>
>>>>
>>>> Please note, the patch is tested on:
>>>> https://urldefense.com/v3/__https://xenbits.xen.org/gitweb/?p=libvirt.git;a=shortlog;h=refs*heads*xen-tested-master__;Ly8!!GF_29dbcQIUBPA!2GtOGVnLefUlZqkD4BAl9lVy82Avr_zPfztssIJ0qIBO8m8GTnSS2n2U6MpS0I2MhBxaE0EA9-GqbnlB-bTeUkf24g$ [xenbits[.]xen[.]org]
>>>>
>>>> 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://urldefense.com/v3/__https://xenbits.xen.org/gitweb/?p=xen.git;a=commit;f=tools*libs*light*libxl_types.idl;h=66dd1c62b2a3c707bd5c55750d10a8223fbd577f__;Ly8v!!GF_29dbcQIUBPA!2GtOGVnLefUlZqkD4BAl9lVy82Avr_zPfztssIJ0qIBO8m8GTnSS2n2U6MpS0I2MhBxaE0EA9-GqbnlB-bR-mL3uGQ$ [xenbits[.]xen[.]org]
>>>
>>>
>>> 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.
> I understand that but what can we do here is to disable -Werror so that
> the commit can land in master. And then merge this libvirt fix. Does
> that work for you?

Michal,
thank you for the review and attempts to find a compromise. As I 
understand the things are bit more complicated, what they look like at 
the first glance (Julien, please correct me if I am wrong). I think, the 
proposed solution perfectly worked for us if Osstest would use actual 
libvirt's master to test against. But Osstest still uses old master 
located at (because it hasn't been adapted yet to new build system using 
Meson):

https://xenbits.xen.org/gitweb/?p=libvirt.git;a=shortlog;h=refs/heads/xen-tested-master

So, I am afraid, any actions in libvirt's master won't immediately solve 
the current situation at the our side. Besides getting the current (or 
alternative) patch merged into the libvirt we will need to fix Osstest 
(update the libvirt build script).

The discussion how to resolve the current situation is in progress now [2]:

[1] 
https://xenbits.xen.org/gitweb/?p=xen.git;a=commit;f=tools/libs/light/libxl_types.idl;h=66dd1c62b2a3c707bd5c55750d10a8223fbd577f
[2] 
https://lore.kernel.org/xen-devel/db39670c-7e36-2cf5-a87b-92d10d3aac18@xen.org/

>
> Michal
>
-- 
Regards,

Oleksandr Tyshchenko