[libvirt] [PATCH] domain_conf: Do not use USB by default for <input> devices on s390x
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@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
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@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@redhat.com> and pushed. Michal
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@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
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@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
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
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
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@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@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
participants (4)
-
Andrea Bolognani -
Ján Tomko -
Michal Privoznik -
Thomas Huth