2:30 a.m.
When trying to specify an input device on s390x without bus like this:
<input type='keyboard'/>
... then libvirt currently complains:
error: unsupported configuration: USB is disabled for this domain,
but USB devices are present in the domain XML
This is somewhat confusing since the user did not specify an USB
device here. Since USB is not available on s390x, we should default
to the "virtio" bus here instead.
Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1790189
Signed-off-by: Thomas Huth <thuth(a)redhat.com>
---
src/conf/domain_conf.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 1290241923..c3761b0f45 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -13428,6 +13428,8 @@ virDomainInputDefParseXML(virDomainXMLOptionPtr xmlopt,
def->type == VIR_DOMAIN_INPUT_TYPE_KBD) &&
(ARCH_IS_X86(dom->os.arch) || dom->os.arch == VIR_ARCH_NONE)) {
def->bus = VIR_DOMAIN_INPUT_BUS_PS2;
+ } else if (ARCH_IS_S390(dom->os.arch)) {
+ def->bus = VIR_DOMAIN_INPUT_BUS_VIRTIO;
} else {
def->bus = VIR_DOMAIN_INPUT_BUS_USB;
}
--
2.18.1
6:48 a.m.
On 1/13/20 9:30 AM, Thomas Huth wrote:
When trying to specify an input device on s390x without bus like
this:
<input type='keyboard'/>
... then libvirt currently complains:
error: unsupported configuration: USB is disabled for this domain,
but USB devices are present in the domain XML
This is somewhat confusing since the user did not specify an USB
device here. Since USB is not available on s390x, we should default
to the "virtio" bus here instead.
Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1790189
Signed-off-by: Thomas Huth <thuth(a)redhat.com>
---
src/conf/domain_conf.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 1290241923..c3761b0f45 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -13428,6 +13428,8 @@ virDomainInputDefParseXML(virDomainXMLOptionPtr xmlopt,
def->type == VIR_DOMAIN_INPUT_TYPE_KBD) &&
(ARCH_IS_X86(dom->os.arch) || dom->os.arch == VIR_ARCH_NONE)) {
def->bus = VIR_DOMAIN_INPUT_BUS_PS2;
+ } else if (ARCH_IS_S390(dom->os.arch)) {
+ def->bus = VIR_DOMAIN_INPUT_BUS_VIRTIO;
} else {
def->bus = VIR_DOMAIN_INPUT_BUS_USB;
}
Reviewed-by: Michal Privoznik <mprivozn(a)redhat.com>
and pushed.
Michal
7:06 a.m.
On Mon, 2020-01-13 at 13:48 +0100, Michal Privoznik wrote:
On 1/13/20 9:30 AM, Thomas Huth wrote:
> +++ b/src/conf/domain_conf.c
> @@ -13428,6 +13428,8 @@ virDomainInputDefParseXML(virDomainXMLOptionPtr xmlopt,
> def->type == VIR_DOMAIN_INPUT_TYPE_KBD) &&
> (ARCH_IS_X86(dom->os.arch) || dom->os.arch ==
VIR_ARCH_NONE)) {
> def->bus = VIR_DOMAIN_INPUT_BUS_PS2;
> + } else if (ARCH_IS_S390(dom->os.arch)) {
> + def->bus = VIR_DOMAIN_INPUT_BUS_VIRTIO;
> } else {
> def->bus = VIR_DOMAIN_INPUT_BUS_USB;
> }
Reviewed-by: Michal Privoznik <mprivozn(a)redhat.com>
and pushed.
I don't believe either this or the other patch posted by Thomas
should have been pushed during the freeze period. I won't ask you
to revert them, but please refrain from pushing further changes
unless 6.0.0 would be utterly broken without them.
--
Andrea Bolognani / Red Hat / Virtualization
8:02 a.m.
On 1/13/20 2:06 PM, Andrea Bolognani wrote:
On Mon, 2020-01-13 at 13:48 +0100, Michal Privoznik wrote:
> On 1/13/20 9:30 AM, Thomas Huth wrote:
>> +++ b/src/conf/domain_conf.c
>> @@ -13428,6 +13428,8 @@ virDomainInputDefParseXML(virDomainXMLOptionPtr xmlopt,
>> def->type == VIR_DOMAIN_INPUT_TYPE_KBD) &&
>> (ARCH_IS_X86(dom->os.arch) || dom->os.arch ==
VIR_ARCH_NONE)) {
>> def->bus = VIR_DOMAIN_INPUT_BUS_PS2;
>> + } else if (ARCH_IS_S390(dom->os.arch)) {
>> + def->bus = VIR_DOMAIN_INPUT_BUS_VIRTIO;
>> } else {
>> def->bus = VIR_DOMAIN_INPUT_BUS_USB;
>> }
>
> Reviewed-by: Michal Privoznik <mprivozn(a)redhat.com>
>
> and pushed.
I don't believe either this or the other patch posted by Thomas
should have been pushed during the freeze period. I won't ask you
to revert them, but please refrain from pushing further changes
unless 6.0.0 would be utterly broken without them.
I thought that freeze period is for us for merge fixes (and this is
one). I believe this patch (and the other too) has no impact on non-s390
arches AND fixes 6.0.0 for the s390.
And for "utterly broken" - I don't think that's the rule per se. I think
we need to evaluate each patch individually.
Michal
9:08 a.m.
On Mon, 2020-01-13 at 15:02 +0100, Michal Privoznik wrote:
On 1/13/20 2:06 PM, Andrea Bolognani wrote:
> I don't believe either this or the other patch posted by Thomas
> should have been pushed during the freeze period. I won't ask you
> to revert them, but please refrain from pushing further changes
> unless 6.0.0 would be utterly broken without them.
I thought that freeze period is for us for merge fixes (and this is
one). I believe this patch (and the other too) has no impact on non-s390
arches AND fixes 6.0.0 for the s390.
And for "utterly broken" - I don't think that's the rule per se. I
think
we need to evaluate each patch individually.
The way I see it, the freeze period is intended to stabilize libvirt
for the upcoming release; as such, changes merged during freeze
should ideally be exceedingly small and targeted.
Of course it's always a trade-off, and whether the positive outcome
outweights the risk of potentially introducing more issues is to be
decided on a case-by-case basis.
Is the patch fixing an issue that was introduced in this release?
Then it's probably worth merging it, because doing so avoids the
situation where we release known-broken software. Is it fixing a
long-standing issue, as is the case here? Then it can probably wait
until the next release.
I feel like this conversation has happened a number of times on the
list already. Perhaps it would be a good idea to try and converge to
a set of accepted guidelines that we can add to our existing
contributor-oriented documentation?
--
Andrea Bolognani / Red Hat / Virtualization
9:25 a.m.
On 1/13/20 4:08 PM, Andrea Bolognani wrote:
On Mon, 2020-01-13 at 15:02 +0100, Michal Privoznik wrote:
> On 1/13/20 2:06 PM, Andrea Bolognani wrote:
>> I don't believe either this or the other patch posted by Thomas
>> should have been pushed during the freeze period. I won't ask you
>> to revert them, but please refrain from pushing further changes
>> unless 6.0.0 would be utterly broken without them.
>
> I thought that freeze period is for us for merge fixes (and this is
> one). I believe this patch (and the other too) has no impact on non-s390
> arches AND fixes 6.0.0 for the s390.
>
> And for "utterly broken" - I don't think that's the rule per se. I
think
> we need to evaluate each patch individually.
The way I see it, the freeze period is intended to stabilize libvirt
for the upcoming release; as such, changes merged during freeze
should ideally be exceedingly small and targeted.
This was exactly my reasoning when I decided to push it:
1 file changed, 2 insertions(+)
10 files changed, 12 insertions(+), 10 deletions(-)
Of course it's always a trade-off, and whether the positive outcome
outweights the risk of potentially introducing more issues is to be
decided on a case-by-case basis.
Is the patch fixing an issue that was introduced in this release?
Then it's probably worth merging it, because doing so avoids the
situation where we release known-broken software. Is it fixing a
long-standing issue, as is the case here? Then it can probably wait
until the next release.
If the fix was large or risky, then certainly wait another release
cycle. But I don't think that's the case here. But I can revert the
patches and merge them after the release, if that makes you feel better
about our release. I don't care that much.
I feel like this conversation has happened a number of times on the
list already. Perhaps it would be a good idea to try and converge to
a set of accepted guidelines that we can add to our existing
contributor-oriented documentation?
What we can have is a different branching model. We could have a master
that is always ready to accept patches and individual releases would
just branch from the master.
Michal
11:18 p.m.
On Mon, Jan 13, 2020 at 02:06:21PM +0100, Andrea Bolognani wrote:
On Mon, 2020-01-13 at 13:48 +0100, Michal Privoznik wrote:
> On 1/13/20 9:30 AM, Thomas Huth wrote:
> > +++ b/src/conf/domain_conf.c
> > @@ -13428,6 +13428,8 @@ virDomainInputDefParseXML(virDomainXMLOptionPtr xmlopt,
> > def->type == VIR_DOMAIN_INPUT_TYPE_KBD) &&
> > (ARCH_IS_X86(dom->os.arch) || dom->os.arch ==
VIR_ARCH_NONE)) {
> > def->bus = VIR_DOMAIN_INPUT_BUS_PS2;
> > + } else if (ARCH_IS_S390(dom->os.arch)) {
> > + def->bus = VIR_DOMAIN_INPUT_BUS_VIRTIO;
> > } else {
> > def->bus = VIR_DOMAIN_INPUT_BUS_USB;
> > }
>
> Reviewed-by: Michal Privoznik <mprivozn(a)redhat.com>
>
> and pushed.
I don't believe either this or the other patch posted by Thomas
should have been pushed during the freeze period. I won't ask you
to revert them, but please refrain from pushing further changes
unless 6.0.0 would be utterly broken without them.
The feature freeze is for refraining from pushing features.
Even if the bug was present for a long time, there's no need to have
yet-another-release when we know it's broken and the fix is simple.
Jano
--
Andrea Bolognani / Red Hat / Virtualization
--
libvir-list mailing list
libvir-list(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
1774
days inactive
1775
days old
6 comments
4 participants
participants (4)
-
Andrea Bolognani -
Ján Tomko -
Michal Privoznik -
Thomas Huth