Re: [libvirt] [PATCH v3 0/4] qemu: fix type of default video device
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(a)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