10:50 a.m.
From: Oleksandr Tyshchenko <oleksandr_tyshchenko(a)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.
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....
[2] https://lore.kernel.org/xen-devel/E1oHEQO-0008GA-Uo@osstest.test-lab.xenp...
Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko(a)epam.com>
---
Cc: Julien Grall <julien(a)xen.org>
Cc: Anthony PERARD <anthony.perard(a)citrix.com>
Cc: Michal Privoznik <mprivozn(a)redhat.com>
Please note, the patch is tested on:
https://xenbits.xen.org/gitweb/?p=libvirt.git;a=shortlog;h=refs/heads/xen...
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(+)
diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
index aa3d7925ec..526f0b2b08 100644
--- a/src/libxl/libxl_conf.c
+++ b/src/libxl/libxl_conf.c
@@ -1240,6 +1240,10 @@ libxlUpdateDiskDef(virDomainDiskDef *l_disk, libxl_device_disk
*x_disk)
driver = "phy";
break;
case LIBXL_DISK_BACKEND_UNKNOWN:
+#ifdef LIBXL_HAVE_DEVICE_DISK_SPECIFICATION
+ case LIBXL_DISK_BACKEND_STANDALONE:
+#endif
+ default:
break;
}
if (driver)
diff --git a/src/libxl/xen_xl.c b/src/libxl/xen_xl.c
index 4de4e3140f..6919325623 100644
--- a/src/libxl/xen_xl.c
+++ b/src/libxl/xen_xl.c
@@ -715,6 +715,9 @@ xenParseXLDisk(virConf *conf, virDomainDef *def)
virDomainDiskSetDriver(disk, "phy");
virDomainDiskSetType(disk, VIR_STORAGE_TYPE_BLOCK);
break;
+#ifdef LIBXL_HAVE_DEVICE_DISK_SPECIFICATION
+ case LIBXL_DISK_BACKEND_STANDALONE:
+#endif
default:
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("disk backend not supported: %s"),
--
2.25.1
3:23 a.m.
On 7/29/22 17:50, Oleksandr Tyshchenko wrote:
From: Oleksandr Tyshchenko <oleksandr_tyshchenko(a)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. 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....
[2] https://lore.kernel.org/xen-devel/E1oHEQO-0008GA-Uo@osstest.test-lab.xenp...
Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko(a)epam.com>
---
Cc: Julien Grall <julien(a)xen.org>
Cc: Anthony PERARD <anthony.perard(a)citrix.com>
Cc: Michal Privoznik <mprivozn(a)redhat.com>
Please note, the patch is tested on:
https://xenbits.xen.org/gitweb/?p=libvirt.git;a=shortlog;h=refs/heads/xen...
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/lib...
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).
Michal
3:51 a.m.
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(a)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?
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@gmail....
> [2]
https://lore.kernel.org/xen-devel/E1oHEQO-0008GA-Uo@osstest.test-lab.xenp...
>
> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko(a)epam.com>
> ---
> Cc: Julien Grall <julien(a)xen.org>
> Cc: Anthony PERARD <anthony.perard(a)citrix.com>
> Cc: Michal Privoznik <mprivozn(a)redhat.com>
>
> Please note, the patch is tested on:
>
https://xenbits.xen.org/gitweb/?p=libvirt.git;a=shortlog;h=refs/heads/xen...
> 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/lib...
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
4:36 a.m.
On Mon, Aug 01, 2022 at 09:51:11AM +0100, 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(a)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?
Yes & no, but mostly yes.
You can choose to build with -Werror or not. If building from .git
then it defaults to enabled, but can be disabled if desired.
Generally we want to see errors triggered from new enums arriving,
as it can be a sign that libvirt code needs a semantic change in
order to continue operating correctly. It isn't always correct
to assume that the 'default' case gives the correct behaviour.
With 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 :|
6:39 a.m.
On Mon, Aug 01, 2022 at 10:36:07AM +0100, Daniel P. Berrangé wrote:
Generally we want to see errors triggered from new enums arriving,
as it can be a sign that libvirt code needs a semantic change in
order to continue operating correctly. It isn't always correct
to assume that the 'default' case gives the correct behaviour.
Isn't that the exact purpose of 'default' label? If use of 'default'
means "any of the other 5 specific values, but lets save some characters
to not name them explicitly", then IMHO better to name them
explicitly...
I can see a value of -Werror=switch-enum when adding new (internal) enum
value, to find all the cases where code needs to be adjusted, but even
then a grep is probably sufficient enough. On the other hand, if there
are cases where indeed all the values of (internal API) enum needs to be
handled explicitly, maybe simply omit 'default' label and use
-Werror=switch?
Anyway, if tracking all the enums values of all the used 3rd-party APIs
is desirable (like, noticing when libxl adds new disk type), then it
probably should be a separate CI job, not the default devel build.
Otherwise breakages like this will happen from time to time, and will
be annoyed for people on involved in specific code part at all.
As a short term fix, maybe Xen's CI can build libvirt with
-Wno-error=switch-enum?
--
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
8:39 a.m.
On Tue, Aug 02, 2022 at 01:39:31PM +0200, Marek Marczykowski-Górecki wrote:
On Mon, Aug 01, 2022 at 10:36:07AM +0100, Daniel P. Berrangé wrote:
> Generally we want to see errors triggered from new enums arriving,
> as it can be a sign that libvirt code needs a semantic change in
> order to continue operating correctly. It isn't always correct
> to assume that the 'default' case gives the correct behaviour.
Isn't that the exact purpose of 'default' label? If use of 'default'
means "any of the other 5 specific values, but lets save some characters
to not name them explicitly", then IMHO better to name them
explicitly...
I can see a value of -Werror=switch-enum when adding new (internal) enum
value, to find all the cases where code needs to be adjusted, but even
then a grep is probably sufficient enough. On the other hand, if there
are cases where indeed all the values of (internal API) enum needs to be
handled explicitly, maybe simply omit 'default' label and use
-Werror=switch?
Anyway, if tracking all the enums values of all the used 3rd-party APIs
is desirable (like, noticing when libxl adds new disk type), then it
probably should be a separate CI job, not the default devel build.
Otherwise breakages like this will happen from time to time, and will
be annoyed for people on involved in specific code part at all.
As a short term fix, maybe Xen's CI can build libvirt with
-Wno-error=switch-enum?
I think makes sense for a 3rd party CI todo that, since these warnings
are primarily targetted at libvirt upstream maintainers, so that we
catch problems before code is committed.
With 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 :|
5:08 a.m.
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(a)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?
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@gmail....
>>
>> [2]
>>
https://lore.kernel.org/xen-devel/E1oHEQO-0008GA-Uo@osstest.test-lab.xenp...
>>
>>
>> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko(a)epam.com>
>> ---
>> Cc: Julien Grall <julien(a)xen.org>
>> Cc: Anthony PERARD <anthony.perard(a)citrix.com>
>> Cc: Michal Privoznik <mprivozn(a)redhat.com>
>>
>> Please note, the patch is tested on:
>>
https://xenbits.xen.org/gitweb/?p=libvirt.git;a=shortlog;h=refs/heads/xen...
>>
>> 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/lib...
>
>
> 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
6:33 a.m.
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(a)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/20220716163...
[lore[.]kernel[.]org]
>>>
>>> [2]
>>>
https://urldefense.com/v3/__https://lore.kernel.org/xen-devel/E1oHEQO-000...
[lore[.]kernel[.]org]
>>>
>>>
>>> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko(a)epam.com>
>>> ---
>>> Cc: Julien Grall <julien(a)xen.org>
>>> Cc: Anthony PERARD <anthony.perard(a)citrix.com>
>>> Cc: Michal Privoznik <mprivozn(a)redhat.com>
>>>
>>> Please note, the patch is tested on:
>>>
https://urldefense.com/v3/__https://xenbits.xen.org/gitweb/?p=libvirt.git...
[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=c...
[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...
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/lib...
[2]
https://lore.kernel.org/xen-devel/db39670c-7e36-2cf5-a87b-92d10d3aac18@xe...
Michal
--
Regards,
Oleksandr Tyshchenko
12:04 p.m.
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(a)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....
>>>
>>> [2]
>>>
https://lore.kernel.org/xen-devel/E1oHEQO-0008GA-Uo@osstest.test-lab.xenp...
>>>
>>>
>>> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko(a)epam.com>
>>> ---
>>> Cc: Julien Grall <julien(a)xen.org>
>>> Cc: Anthony PERARD <anthony.perard(a)citrix.com>
>>> Cc: Michal Privoznik <mprivozn(a)redhat.com>
>>>
>>> Please note, the patch is tested on:
>>>
https://xenbits.xen.org/gitweb/?p=libvirt.git;a=shortlog;h=refs/heads/xen...
>>>
>>> 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/lib...
>>
>>
>> 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
8:20 a.m.
On Mon, Aug 01, 2022 at 06:04:38PM +0100, Julien Grall wrote:
On 01/08/2022 11:08, Michal Prívozník wrote:
> 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).
Actually, if an application defines LIBXL_API_VERSION, like libvirt
does, we expect the application to still build against the latest
version of libxl. But I don't think that should prevent us from adding
new enum values.
Cheers,
--
Anthony PERARD
4:57 a.m.
On Mon, Aug 01, 2022 at 10:23:48AM +0200, Michal Prívozník wrote:
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/lib...
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).
Hi Michal,
Now that the commit is "master", do you think this libvirt patch could
be committed?
Thanks,
--
Anthony PERARD
9:07 a.m.
On 8/19/22 11:57, Anthony PERARD wrote:
On Mon, Aug 01, 2022 at 10:23:48AM +0200, Michal Prívozník wrote:
> 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/lib...
>
> 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).
Hi Michal,
Now that the commit is "master", do you think this libvirt patch could
be committed?
Indeed. Let me do that right now.
Michal
9:06 a.m.
On 7/29/22 17:50, Oleksandr Tyshchenko wrote:
From: Oleksandr Tyshchenko <oleksandr_tyshchenko(a)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.
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....
[2] https://lore.kernel.org/xen-devel/E1oHEQO-0008GA-Uo@osstest.test-lab.xenp...
Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko(a)epam.com>
---
Cc: Julien Grall <julien(a)xen.org>
Cc: Anthony PERARD <anthony.perard(a)citrix.com>
Cc: Michal Privoznik <mprivozn(a)redhat.com>
Please note, the patch is tested on:
https://xenbits.xen.org/gitweb/?p=libvirt.git;a=shortlog;h=refs/heads/xen...
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(+)
Now that the libxl patch is merged into the master:
https://xenbits.xen.org/gitweb/?p=xen.git;a=commit;h=66dd1c62b2a3c707bd5c...
I can merge this.
Reviewed-by: Michal Privoznik <mprivozn(a)redhat.com>
Michal
822
days inactive
846
days old
12 comments
7 participants
participants (7)
-
Anthony PERARD -
Daniel P. Berrangé -
Julien Grall -
Marek Marczykowski-Górecki -
Michal Prívozník -
Oleksandr Tyshchenko -
Oleksandr Tyshchenko