[libvirt] [PATCH v3 0/4] qemu: fix type of default video device

Pavel Mores posted 4 patches 4 years, 4 months ago
Test syntax-check failed
Failed in applying to current master (apply log)
src/qemu/qemu_domain.c                        | 37 ++++++++++------
.../default-video-type-aarch64.xml            | 16 +++++++
.../default-video-type-ppc64.xml              | 16 +++++++
.../default-video-type-riscv64.xml            | 16 +++++++
.../default-video-type-s390x.xml              | 16 +++++++
.../default-video-type-x86_64-caps-test-0.xml | 17 ++++++++
.../default-video-type-x86_64-caps-test-1.xml | 17 ++++++++
tests/qemuxml2argvtest.c                      |  1 +
...ault-video-type-aarch64.aarch64-latest.xml | 42 +++++++++++++++++++
.../default-video-type-ppc64.ppc64-latest.xml | 31 ++++++++++++++
...ault-video-type-riscv64.riscv64-latest.xml | 39 +++++++++++++++++
.../default-video-type-s390x.s390x-latest.xml | 32 ++++++++++++++
.../default-video-type-x86_64-caps-test-0.xml | 31 ++++++++++++++
.../default-video-type-x86_64-caps-test-1.xml | 31 ++++++++++++++
tests/qemuxml2xmltest.c                       | 10 ++++-
15 files changed, 339 insertions(+), 13 deletions(-)
create mode 100644 tests/qemuxml2argvdata/default-video-type-aarch64.xml
create mode 100644 tests/qemuxml2argvdata/default-video-type-ppc64.xml
create mode 100644 tests/qemuxml2argvdata/default-video-type-riscv64.xml
create mode 100644 tests/qemuxml2argvdata/default-video-type-s390x.xml
create mode 100644 tests/qemuxml2argvdata/default-video-type-x86_64-caps-test-0.xml
create mode 100644 tests/qemuxml2argvdata/default-video-type-x86_64-caps-test-1.xml
create mode 100644 tests/qemuxml2xmloutdata/default-video-type-aarch64.aarch64-latest.xml
create mode 100644 tests/qemuxml2xmloutdata/default-video-type-ppc64.ppc64-latest.xml
create mode 100644 tests/qemuxml2xmloutdata/default-video-type-riscv64.riscv64-latest.xml
create mode 100644 tests/qemuxml2xmloutdata/default-video-type-s390x.s390x-latest.xml
create mode 100644 tests/qemuxml2xmloutdata/default-video-type-x86_64-caps-test-0.xml
create mode 100644 tests/qemuxml2xmloutdata/default-video-type-x86_64-caps-test-1.xml
[libvirt] [PATCH v3 0/4] qemu: fix type of default video device
Posted by Pavel Mores 4 years, 4 months ago
This new version mostly integrates Cole's comments about the second version.
Refactoring and behaviour change are now separate commits.  Tests succeed for
every individual patch in the series.

Pavel Mores (4):
  qemu: default video device type selection algoritm moved into its own
    function
  qemu: prepare existing test for change of the default video device
    type
  qemu: the actual change of default video devide type selection
    algorithm
  qemu: added tests of the new default video type selection algorithm

 src/qemu/qemu_domain.c                        | 37 ++++++++++------
 .../default-video-type-aarch64.xml            | 16 +++++++
 .../default-video-type-ppc64.xml              | 16 +++++++
 .../default-video-type-riscv64.xml            | 16 +++++++
 .../default-video-type-s390x.xml              | 16 +++++++
 .../default-video-type-x86_64-caps-test-0.xml | 17 ++++++++
 .../default-video-type-x86_64-caps-test-1.xml | 17 ++++++++
 tests/qemuxml2argvtest.c                      |  1 +
 ...ault-video-type-aarch64.aarch64-latest.xml | 42 +++++++++++++++++++
 .../default-video-type-ppc64.ppc64-latest.xml | 31 ++++++++++++++
 ...ault-video-type-riscv64.riscv64-latest.xml | 39 +++++++++++++++++
 .../default-video-type-s390x.s390x-latest.xml | 32 ++++++++++++++
 .../default-video-type-x86_64-caps-test-0.xml | 31 ++++++++++++++
 .../default-video-type-x86_64-caps-test-1.xml | 31 ++++++++++++++
 tests/qemuxml2xmltest.c                       | 10 ++++-
 15 files changed, 339 insertions(+), 13 deletions(-)
 create mode 100644 tests/qemuxml2argvdata/default-video-type-aarch64.xml
 create mode 100644 tests/qemuxml2argvdata/default-video-type-ppc64.xml
 create mode 100644 tests/qemuxml2argvdata/default-video-type-riscv64.xml
 create mode 100644 tests/qemuxml2argvdata/default-video-type-s390x.xml
 create mode 100644 tests/qemuxml2argvdata/default-video-type-x86_64-caps-test-0.xml
 create mode 100644 tests/qemuxml2argvdata/default-video-type-x86_64-caps-test-1.xml
 create mode 100644 tests/qemuxml2xmloutdata/default-video-type-aarch64.aarch64-latest.xml
 create mode 100644 tests/qemuxml2xmloutdata/default-video-type-ppc64.ppc64-latest.xml
 create mode 100644 tests/qemuxml2xmloutdata/default-video-type-riscv64.riscv64-latest.xml
 create mode 100644 tests/qemuxml2xmloutdata/default-video-type-s390x.s390x-latest.xml
 create mode 100644 tests/qemuxml2xmloutdata/default-video-type-x86_64-caps-test-0.xml
 create mode 100644 tests/qemuxml2xmloutdata/default-video-type-x86_64-caps-test-1.xml

-- 
2.21.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v3 0/4] qemu: fix type of default video device
Posted by Cole Robinson 4 years, 4 months ago
On 11/25/19 5:54 AM, Pavel Mores wrote:
> This new version mostly integrates Cole's comments about the second version.
> Refactoring and behaviour change are now separate commits.  Tests succeed for
> every individual patch in the series.
> 
> Pavel Mores (4):
>   qemu: default video device type selection algoritm moved into its own
>     function
>   qemu: prepare existing test for change of the default video device
>     type
>   qemu: the actual change of default video devide type selection
>     algorithm
>   qemu: added tests of the new default video type selection algorithm

Reviewed-by: Cole Robinson <crobinso@redhat.com>

and pushed now.

patch #3 still had some style issues I pointed out in the previous
review: using 'else' when it isn't necessary, because every block
returns. Better to use the style of qemuDomainDefaultNetModel which
saves having to do any nested logic. I'll be happy to review a follow up
cleanup patch for that

Minor bit: I would have squashed patch #2 + #3 together, maybe only
separating them if there was lots of test suite churn needed, but I'm
not sure if others agree.

Thanks,
Cole

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v3 0/4] qemu: fix type of default video device
Posted by Pavel Mores 4 years, 4 months ago
On Mon, Nov 25, 2019 at 08:54:43AM -0500, Cole Robinson wrote:
> On 11/25/19 5:54 AM, Pavel Mores wrote:
> > This new version mostly integrates Cole's comments about the second version.
> > Refactoring and behaviour change are now separate commits.  Tests succeed for
> > every individual patch in the series.
> > 
> > Pavel Mores (4):
> >   qemu: default video device type selection algoritm moved into its own
> >     function
> >   qemu: prepare existing test for change of the default video device
> >     type
> >   qemu: the actual change of default video devide type selection
> >     algorithm
> >   qemu: added tests of the new default video type selection algorithm
> 
> Reviewed-by: Cole Robinson <crobinso@redhat.com>
> 
> and pushed now.
> 
> patch #3 still had some style issues I pointed out in the previous
> review: using 'else' when it isn't necessary, because every block
> returns. Better to use the style of qemuDomainDefaultNetModel which
> saves having to do any nested logic. I'll be happy to review a follow up
> cleanup patch for that

Oh, now I see I changed my own additions but overlooked the style of
pre-existing code.  Sorry about that - I guess I don't have an eye for this as
personally I'd probably slightly prefer the else-based style.

> Minor bit: I would have squashed patch #2 + #3 together, maybe only
> separating them if there was lots of test suite churn needed, but I'm
> not sure if others agree.
> 
> Thanks,
> Cole
> 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v3 0/4] qemu: fix type of default video device
Posted by Cole Robinson 4 years, 4 months ago
On 11/25/19 9:40 AM, Pavel Mores wrote:
> On Mon, Nov 25, 2019 at 08:54:43AM -0500, Cole Robinson wrote:
>> On 11/25/19 5:54 AM, Pavel Mores wrote:
>>> This new version mostly integrates Cole's comments about the second version.
>>> Refactoring and behaviour change are now separate commits.  Tests succeed for
>>> every individual patch in the series.
>>>
>>> Pavel Mores (4):
>>>   qemu: default video device type selection algoritm moved into its own
>>>     function
>>>   qemu: prepare existing test for change of the default video device
>>>     type
>>>   qemu: the actual change of default video devide type selection
>>>     algorithm
>>>   qemu: added tests of the new default video type selection algorithm
>>
>> Reviewed-by: Cole Robinson <crobinso@redhat.com>
>>
>> and pushed now.
>>
>> patch #3 still had some style issues I pointed out in the previous
>> review: using 'else' when it isn't necessary, because every block
>> returns. Better to use the style of qemuDomainDefaultNetModel which
>> saves having to do any nested logic. I'll be happy to review a follow up
>> cleanup patch for that
> 
> Oh, now I see I changed my own additions but overlooked the style of
> pre-existing code.  Sorry about that - I guess I don't have an eye for this as
> personally I'd probably slightly prefer the else-based style.
> 

The main issue with the code as implemented is that the third block is
indented under an 'else' when it doesn't need to be. If we followed that
pattern to the extreme the code would look like

if (condition1) {
    return foo;
} else {
    if (condition2) {
        return bar;
    } else {
        if (condition3)
            return baz;
    }
}

instead of

if (condition1)
    return foo;
if (condition2)
    return bar;
if (condition3)
    return baz;

Thanks,
Cole

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v3 0/4] qemu: fix type of default video device
Posted by Pavel Mores 4 years, 4 months ago
On Mon, Nov 25, 2019 at 10:04:58AM -0500, Cole Robinson wrote:
> On 11/25/19 9:40 AM, Pavel Mores wrote:
> > On Mon, Nov 25, 2019 at 08:54:43AM -0500, Cole Robinson wrote:
> >> On 11/25/19 5:54 AM, Pavel Mores wrote:
> >>> This new version mostly integrates Cole's comments about the second version.
> >>> Refactoring and behaviour change are now separate commits.  Tests succeed for
> >>> every individual patch in the series.
> >>>
> >>> Pavel Mores (4):
> >>>   qemu: default video device type selection algoritm moved into its own
> >>>     function
> >>>   qemu: prepare existing test for change of the default video device
> >>>     type
> >>>   qemu: the actual change of default video devide type selection
> >>>     algorithm
> >>>   qemu: added tests of the new default video type selection algorithm
> >>
> >> Reviewed-by: Cole Robinson <crobinso@redhat.com>
> >>
> >> and pushed now.
> >>
> >> patch #3 still had some style issues I pointed out in the previous
> >> review: using 'else' when it isn't necessary, because every block
> >> returns. Better to use the style of qemuDomainDefaultNetModel which
> >> saves having to do any nested logic. I'll be happy to review a follow up
> >> cleanup patch for that
> > 
> > Oh, now I see I changed my own additions but overlooked the style of
> > pre-existing code.  Sorry about that - I guess I don't have an eye for this as
> > personally I'd probably slightly prefer the else-based style.
> > 
> 
> The main issue with the code as implemented is that the third block is
> indented under an 'else' when it doesn't need to be. If we followed that
> pattern to the extreme the code would look like
> 
> if (condition1) {
>     return foo;
> } else {
>     if (condition2) {
>         return bar;
>     } else {
>         if (condition3)
>             return baz;
>     }
> }
> 
> instead of
> 
> if (condition1)
>     return foo;
> if (condition2)
>     return bar;
> if (condition3)
>     return baz;

I agree that the way the code is now, the flat style is more readable.  I just
feel that the additional structure provided by nesting could come handy if the
algorithm has be changed again in a way that made it impossible to have just a
return in every branch.

If we can reasonably assume that that's not going to happen, and if the
original author doesn't object (cc'ing him), I'm happy to make the change.

Thanks,

	pvl

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list