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

[Michal Prívozník <mprivozn@xxxxxxxxxx>]

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@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?

-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?

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://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.

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