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

[Julien Grall]

Hi Michal,

On 01/08/2022 11:08, Michal Prívozník wrote:
> On 8/1/22 10:51, Julien Grall wrote:
>> 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?

That's correct. Once a patch is merged in staging, we would only revert 
it if there is a breakage that can't be easily solved.

> 
> BTW: every other package that does switch() over libxl_disk_backend enum
> will need this fix.

Indeed. From my understanding, there is an expectation that tools built 
on top of libxl may need some update to work on the latest Xen. I will 
let Anthony (one of the tools maintainers to confirm).

>  >>
>>>   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@gmail.com/
>>>>
>>>> [2]
>>>> https://lore.kernel.org/xen-devel/E1oHEQO-0008GA-Uo@osstest.test-lab.xenproject.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://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.
> 
> 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?

This sounds a sensible plan. Anyone from Xen community has an objection 
this with approach?

Cheers,

-- 
Julien Grall