Re: [libvirt] [PATCH v2 0/4] domain capabilities: Expose firmware auto selection feature

10 Apr 2019

      Hi,

On 04/09/19 16:52, Michal Privoznik wrote:
...
v2 of:
https://www.redhat.com/archives/libvir-list/2019-April/msg00460.html
diff to v1:
- Expose 'secure' too
- Switch to uint64_t for qemuFirmwareGetSupported()
Michal Prívozník (4):
  qemu_firmware: Separate firmware loading into a function
  qemu_firmware: Separate machine and arch matching into a function
  qemu_firmware: Introduce qemuFirmwareGetSupported
  domain capabilities: Expose firmware auto selection feature
docs/formatdomaincaps.html.in                 |  23 +++
 docs/schemas/domaincaps.rng                   |   1 +
 src/conf/domain_capabilities.c                |   3 +
 src/conf/domain_capabilities.h                |   2 +
 src/qemu/qemu_capabilities.c                  |  35 +++-
 src/qemu/qemu_capabilities.h                  |   1 +
 src/qemu/qemu_driver.c                        |   1 +
 src/qemu/qemu_firmware.c                      | 169 ++++++++++++++----
 src/qemu/qemu_firmware.h                      |  10 ++
 tests/Makefile.am                             |   4 +-
 .../qemu_1.7.0.x86_64.xml                     |   7 +
 .../qemu_2.12.0-virt.aarch64.xml              |   6 +
 .../qemu_2.12.0.ppc64.xml                     |   4 +
 .../qemu_2.12.0.s390x.xml                     |   4 +
 .../qemu_2.12.0.x86_64.xml                    |   7 +
 .../qemu_2.6.0-virt.aarch64.xml               |   6 +
 .../qemu_2.6.0.aarch64.xml                    |   4 +
 .../domaincapsschemadata/qemu_2.6.0.ppc64.xml |   4 +
 .../qemu_2.6.0.x86_64.xml                     |   7 +
 .../domaincapsschemadata/qemu_2.7.0.s390x.xml |   4 +
 .../qemu_2.8.0-tcg.x86_64.xml                 |   7 +
 .../domaincapsschemadata/qemu_2.8.0.s390x.xml |   4 +
 .../qemu_2.8.0.x86_64.xml                     |   7 +
 .../qemu_2.9.0-q35.x86_64.xml                 |   8 +
 .../qemu_2.9.0-tcg.x86_64.xml                 |   7 +
 .../qemu_2.9.0.x86_64.xml                     |   7 +
 .../domaincapsschemadata/qemu_3.0.0.s390x.xml |   4 +
 .../qemu_3.1.0.x86_64.xml                     |   7 +
 .../qemu_4.0.0.x86_64.xml                     |   7 +
 tests/domaincapstest.c                        |  16 ++
 tests/qemufirmwaretest.c                      |  72 ++++++++
 31 files changed, 412 insertions(+), 36 deletions(-)
you didn't push these patch sets to your personal repo, and also didn't
mention the fork-off commits on master. This matters because neither v1
nor v2 applies on top of master now (i.e., on a5e16020907e). So I tried
to correlate the posting timestamps of the cover letters with the commit
dates (not authorship dates) of the recent commits in the git history.
Ultimately I applied your
- v1 on top of fb0d6049cccf ("docs: Remove search.php and all
references", 2019-04-04), and
- v2 on top of c3e1275b6020 ("rpc: Refactor cleanup paths in
virNetLibsshAuthenticatePassword", 2019-04-09).

Then (because I have very little time for reviewing this,
unfortunately), I ran

$ git range-diff master michal_v1 michal_v2

... From that, I have two comments for the testSupportedFW() function:

(1) You still have one instance of:

    ++        expectedInterfaces |= 1 << data->interfaces[i];

Please update the integer constant 1 to 1ULL here as well.

(2) You have an error message in

    ++    if (actualSecure != data->secure) {
    ++        fprintf(stderr,
    ++                "Mismatch in supported secure boot. "
    ++                "Expected %d got %d\n",
    ++                data->secure, actualSecure);
     +        return -1;
     +    }

Please replace

  "Mismatch in supported secure boot. "

with

  "Mismatch in SMM requirement/support. "

(The commit message has been updated correctly already: it says "list of
supported interfaces and SMM feature", so that's OK.)

With (1) and (2) addressed:

Acked-by: Laszlo Ersek <lersek@redhat.com>

(If there are no other updates, I'm fine if you don't post v3 just for
these.)

Thanks
Laszlo