Re: [PATCH 1/4] qemu: Advertise ACPI support for aarch64 guests

[Andrea Bolognani <abologna@xxxxxxxxxx>]

On Mon, 2017-03-27 at 10:26 -0400, John Ferlan wrote:
[...]
> > > Considering on what's coming in patch 2, this would be better as a
> > > virQEMUCapsSetFirmwareCaps? "utility" function...  That way the added
> > > comments in both places referencing the other place could be dropped.
> > 
> > HPET and KVM PIT are not firmware-related, though.
> > 
> > How about I move setting the arch based on the monitor to
> > a separate virQEMUCapsInitQMPArch() and leave only setting
> > the actual arch-dependent capabilities in this function?
> 
> I think if "all" the lines were in a single API it would reduce the
> chance that some future self would have to have to (or be told to) keep
> this in sync with testUpdateQEMUCaps.

Sorry, maybe I was not clear enough: I like your idea
of moving those to a separate function and calling that
function from the test suite instead of duplicating code!
The only thing I'm questioning is the name.

The attached patch should give you an idea of the direction
I'm heading: virQEMUCapsInitQMPArch() would only be called
from the library code, while virQEMUCapsInitArchQMPBasic()
would be called both there and in the test suite.

Does that look reasonable?

-- 
Andrea Bolognani / Red Hat / Virtualization

From 45fb12ad66714cb884e1828d14e8e660fcccba1f Mon Sep 17 00:00:00 2001
From: Andrea Bolognani <abologna@xxxxxxxxxx>
Date: Mon, 27 Mar 2017 16:36:55 +0200
Subject: [PATCH] wip

---
 src/qemu/qemu_capabilities.c | 37 +++++++++++++++++++++++++++----------
 1 file changed, 27 insertions(+), 10 deletions(-)

diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index f51141b..c38df95 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -4177,11 +4177,18 @@ virQEMUCapsInitQMPBasic(virQEMUCapsPtr qemuCaps)
     virQEMUCapsSet(qemuCaps, QEMU_CAPS_DISPLAY);
 }
 
-/* Capabilities that are architecture depending
- * initialized for QEMU.
+
+/**
+ * virQEMUCapsInitQMPArch:
+ * @qemuCaps: QEMU capabilities
+ * @mon: QEMU monitor
+ *
+ * Initialize the architecture for @qemuCaps by asking @mon.
+ *
+ * Returns: 0 on success, <0 on failure
  */
 static int
-virQEMUCapsInitArchQMPBasic(virQEMUCapsPtr qemuCaps,
+virQEMUCapsInitQMPArch(virQEMUCapsPtr qemuCaps,
                             qemuMonitorPtr mon)
 {
     char *archstr = NULL;
@@ -4196,18 +4203,26 @@ virQEMUCapsInitArchQMPBasic(virQEMUCapsPtr qemuCaps,
         goto cleanup;
     }
 
+    ret = 0;
+
+ cleanup:
+    VIR_FREE(archstr);
+    return ret;
+}
+
+
+/* Capabilities that are architecture depending
+ * initialized for QEMU.
+ */
+static void
+virQEMUCapsInitArchQMPBasic(virQEMUCapsPtr qemuCaps)
+{
     /* ACPI/HPET/KVM PIT are x86 specific */
     if (ARCH_IS_X86(qemuCaps->arch)) {
         virQEMUCapsSet(qemuCaps, QEMU_CAPS_NO_ACPI);
         virQEMUCapsSet(qemuCaps, QEMU_CAPS_NO_HPET);
         virQEMUCapsSet(qemuCaps, QEMU_CAPS_NO_KVM_PIT);
     }
-
-    ret = 0;
-
- cleanup:
-    VIR_FREE(archstr);
-    return ret;
 }
 
 
@@ -4432,9 +4447,11 @@ virQEMUCapsInitQMPMonitor(virQEMUCapsPtr qemuCaps,
 
     virQEMUCapsInitQMPBasic(qemuCaps);
 
-    if (virQEMUCapsInitArchQMPBasic(qemuCaps, mon) < 0)
+    if (virQEMUCapsInitQMPArch(qemuCaps, mon) < 0)
         goto cleanup;
 
+    virQEMUCapsInitArchQMPBasic(qemuCaps);
+
     /* USB option is supported v1.3.0 onwards */
     if (qemuCaps->version >= 1003000)
         virQEMUCapsSet(qemuCaps, QEMU_CAPS_MACHINE_USB_OPT);
-- 
2.7.4

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