Code review comment for ~paelzer/ubuntu/+source/qemu:ubuntu-eoan-4.0

Rafael David Tinoco (rafaeldtinoco) wrote :

Alright, I have deviated a bit my review from testing features since I've seen how you're testing generated packages, so I did a basic functional test only. Instead, I have carefully reviewed all historic (and new) patches to make sure things were good, and assumed this would have a better benefit for you.. so here it goes the full version of the file: and I'm highlighting bellow only the things that I would like clarification and/or a fix for:



changelog: - d/qemu-kvm.service: systemd unit to call qemu-kvm-init

=> this was changed in 1:2.10+dfsg-0ubuntu3 (artful)
=> d/qemu-system-common.qemu-kvm.service: systemd unit to call qemu-kvm-init


changelog: - d/qemu-system-common.install: install systemd unit and helper script

=> d/qemu-system-common.install: install helper script, man pages and docs

=> systemd units are installed by debian/rules using debhelper now:
dh_installsystemd -a -pqemu-system-common --no-restart-on-upgrade --name=qemu-kvm
dh_installsystemd -a -pqemu-guest-agent --no-start --no-enable


changelog: - d/rules: install /etc/default/qemu-kvm

=> d/rules: do not install /etc/default/qemu-kvm anymore (line is commented)


changelog - d/p/ubuntu/lp-1761372-*: provide pseries-bionic-2.11-sxxm type as convenience with all meltdown/spectre workarounds enabled by default. (LP: 1761372).

=> I believe you dropped debian/patches/ubuntu/lp-1761372-* files.

=> d/p/ubuntu/define-ubuntu-machine-types.patch: provide pseries-bionic-2.11-sxxm type as convenience with all meltdown/spectre workarounds enabled by default. (LP: 1761372).


changelog: - arch aware kvm wrappers

=> should this s390x (w/ numa) and qemu-kvm wrapper still be mentioned here ?


changelog: - d/control: update VCS links (updated to match latest Ubuntu)

=> why pointing to salsa vcs or ubuntu-disco-3.1 when you're doing 4.0 from upstream ?


changelog: - d/rules: build s390x-netboot.img with upstream Makefile

=> extra "x" after s390
=> d/rules: build s390-netboot.img with upstream Makefile


changelog: - remove /dev/kvm permission handling (moved to systemd 239-6) (#892945)

=> should this still be mentioned ?
=> we're relying on systemd udev /dev/kvm rule for some time now.


changelog: - pc_compat_2_0 got a _fn suffix and slight changes

=> can we refer d/p/u/define-ubuntu-machine-types.patch being changed here ?

=> its clear for you, i took me couple of minutes to realize what/how you've done

=> i confess i'm not a big fan of this big patch, even if they're all related to mach types (new ones, or old ones adapted like -hpb types and GlobalProperties). maybe in the next merge (release/etc) this could be split into d/p/u/mach/* or something else ? to make it easier to correlate upstream cases like the irqchip split being default, to ubuntu bugs being addressed, or even machine types being created because of CVEs only.


changelog: - d/p/ubuntu/lp-1790901-partial-SLOF-for-s390x-netboot.patch: update to SLOF of qemu 4.0

=> already mentioned before (not with 4.0 version)

=> by doing like this, how to correlate between qemu upstream versions and SLOF versions ? this is a similar problem we have on kernel driver <-> firmware relationships the way I see, merges will always bring fun, I'd suggest *at least* a regression test for SLOF if you don't have it yet.


changelog: - d/p/ubuntu/pre-bionic-256k-ipxe-efi-roms.patch

=> already mentioned before


changelog: - d/control*: enable docs (now explcit) and provide new build-dep python3-sphinx

=> very minor: "now explcit" to "now explicit"


changelog: - d/p/ubuntu/linux-user-fix-__NR_semtimedop-undeclared-error.patch: fix i386 build error

=> fix - error is in 5.2-rc1 includes for asm-mips/unistd*, so, by saying i386, you meant cross compilation for mips build errors ? or you wanted mips instead of i386 ?


I'm happy to work with you, your qemu maintenance is amazing. I really enjoyed following your public bugs as I've seen lots of bug investigations and solutions (like the roms load size mismatch, the backport requests).

This list is everything I could see in this review. Hope it serves you.

Best Regards


review: Needs Fixing

« Back to merge proposal