1. Patchew
  2. Libvirt
  3. View series

[libvirt] [PATCH] domain_conf: Do not use USB by default for <input> devices on s390x

Thomas Huth posted 1 patch 4 years, 3 months ago
Test syntax-check failed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/20200113083014.14348-1-thuth@redhat.com
src/conf/domain_conf.c | 2 ++
1 file changed, 2 insertions(+)
[libvirt] [PATCH] domain_conf: Do not use USB by default for <input> devices on s390x
Posted by Thomas Huth 4 years, 3 months ago 
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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] domain_conf: Do not use USB by default for <input> devices on s390x
Posted by Michal Privoznik 4 years, 3 months ago 
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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] domain_conf: Do not use USB by default for <input> devices on s390x
Posted by Andrea Bolognani 4 years, 3 months ago 
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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] domain_conf: Do not use USB by default for <input> devices on s390x
Posted by Michal Privoznik 4 years, 3 months ago 
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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] domain_conf: Do not use USB by default for <input> devices on s390x
Posted by Andrea Bolognani 4 years, 3 months ago 
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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] domain_conf: Do not use USB by default for <input> devices on s390x
Posted by Michal Privoznik 4 years, 3 months ago 
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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] domain_conf: Do not use USB by default for <input> devices on s390x
Posted by Ján Tomko 4 years, 3 months ago 
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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.