diff mbox series

[v2,03/25] qemu: Move <memballoon> validation out of qemu_command.c

Message ID 4125ca8687d60bf72f717b5642adf6cf95739759.1548278585.git.crobinso@redhat.com
State Accepted
Commit 6427bfc8b3bb8c7a6ad55e09f27c0bcc5d1a85bf
Headers show
Series qemu: virtio-{non-}transitional support | expand

Commit Message

Cole Robinson Jan. 23, 2019, 9:32 p.m. UTC 
If we validate that memballoon is NONE|VIRTIO at parse time,
we can drop similar checks elsewhere in the qemu driver

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

---
 src/qemu/qemu_command.c        | 17 ----------------
 src/qemu/qemu_domain.c         | 36 +++++++++++++++++++++++++++++++++-
 src/qemu/qemu_domain_address.c | 10 ++++------
 src/qemu/qemu_driver.c         | 17 +++++++---------
 src/qemu/qemu_process.c        |  3 +--
 5 files changed, 47 insertions(+), 36 deletions(-)

-- 
2.20.1

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

Comments

Andrea Bolognani Jan. 24, 2019, 3:46 p.m. UTC | #1 
On Wed, 2019-01-23 at 16:32 -0500, Cole Robinson wrote:
[...]
> +static int

> +qemuDomainDeviceDefValidateMemballoon(const virDomainMemballoonDef *memballoon,


You could pass

  const virDomainDef *def

too here, as most other qemuDomainDeviceDefValidate*() functions
already do: that would allow you to...

> +                                      virQEMUCapsPtr qemuCaps)

> +{

> +    if (!memballoon ||

> +        memballoon->model == VIR_DOMAIN_MEMBALLOON_MODEL_NONE) {

> +        return 0;

> +    }


... replace this with

  if (!virDomainDefHasMemballoon(def))
      return 0;

which is arguably slightly nicer. But this version works perfectly
fine, so it's entirely up to you whether to do that or not.

[...]
> @@ -2463,11 +2462,10 @@ static int qemuDomainSetMemoryStatsPeriod(virDomainPtr dom, int period,

>      }

>  

>      if (persistentDef) {

> -        if (!persistentDef->memballoon ||

> -            persistentDef->memballoon->model != VIR_DOMAIN_MEMBALLOON_MODEL_VIRTIO) {

> +        if (!virDomainDefHasMemballoon(def)) {


s/def/persistentDef/

With this fixed,

  Reviewed-by: Andrea Bolognani <abologna@redhat.com>


-- 
Andrea Bolognani / Red Hat / Virtualization

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Cole Robinson Jan. 24, 2019, 11:23 p.m. UTC | #2 
On 01/24/2019 10:46 AM, Andrea Bolognani wrote:
> On Wed, 2019-01-23 at 16:32 -0500, Cole Robinson wrote:

> [...]

>> +static int

>> +qemuDomainDeviceDefValidateMemballoon(const virDomainMemballoonDef *memballoon,

> 

> You could pass

> 

>   const virDomainDef *def

> 

> too here, as most other qemuDomainDeviceDefValidate*() functions

> already do: that would allow you to...

> 

>> +                                      virQEMUCapsPtr qemuCaps)

>> +{

>> +    if (!memballoon ||

>> +        memballoon->model == VIR_DOMAIN_MEMBALLOON_MODEL_NONE) {

>> +        return 0;

>> +    }

> 

> ... replace this with

> 

>   if (!virDomainDefHasMemballoon(def))

>       return 0;

> 

> which is arguably slightly nicer. But this version works perfectly

> fine, so it's entirely up to you whether to do that or not.

> 


I agree that would be nicer, but I dug a bit deeper: This code path is
triggered in two major different ways: as a part of validating a full
DomainDef, but also for validating an _individual_ device which hasn't
been added to a DomainDef yet. The latter path is via
virDomainDeviceDefParse which is called in hotplug situations. So to be
completely correct this function can't validate against def->memballoon
because it may not have been set yet.

> [...]

>> @@ -2463,11 +2462,10 @@ static int qemuDomainSetMemoryStatsPeriod(virDomainPtr dom, int period,

>>      }

>>  

>>      if (persistentDef) {

>> -        if (!persistentDef->memballoon ||

>> -            persistentDef->memballoon->model != VIR_DOMAIN_MEMBALLOON_MODEL_VIRTIO) {

>> +        if (!virDomainDefHasMemballoon(def)) {

> 

> s/def/persistentDef/

> 

> With this fixed,

> 




>   Reviewed-by: Andrea Bolognani <abologna@redhat.com>

> 


Nice catch

Thanks,
Cole

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

Patch

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 128cf331b3..2d05b4a93b 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -4046,20 +4046,9 @@  qemuBuildMemballoonCommandLine(virCommandPtr cmd,
 {
     virBuffer buf = VIR_BUFFER_INITIALIZER;
 
-    if (STRPREFIX(def->os.machine, "s390-virtio") &&
-        virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_S390) && def->memballoon)
-        def->memballoon->model = VIR_DOMAIN_MEMBALLOON_MODEL_NONE;
-
     if (!virDomainDefHasMemballoon(def))
         return 0;
 
-    if (def->memballoon->model != VIR_DOMAIN_MEMBALLOON_MODEL_VIRTIO) {
-        virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
-                       _("Memory balloon device type '%s' is not supported by this version of qemu"),
-                       virDomainMemballoonModelTypeToString(def->memballoon->model));
-        return -1;
-    }
-
     if (qemuBuildVirtioDevStr(&buf, "virtio-balloon",
                               def->memballoon->info.type) < 0) {
         goto error;
@@ -4070,12 +4059,6 @@  qemuBuildMemballoonCommandLine(virCommandPtr cmd,
         goto error;
 
     if (def->memballoon->autodeflate != VIR_TRISTATE_SWITCH_ABSENT) {
-        if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_BALLOON_AUTODEFLATE)) {
-            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
-                           _("deflate-on-oom is not supported by this QEMU binary"));
-            goto error;
-        }
-
         virBufferAsprintf(&buf, ",deflate-on-oom=%s",
                           virTristateSwitchTypeToString(def->memballoon->autodeflate));
     }
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 1376819020..a0ab55d5db 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -3481,6 +3481,10 @@  qemuDomainDefAddDefaultDevices(virDomainDefPtr def,
         def->memballoon = memballoon;
     }
 
+    if (STRPREFIX(def->os.machine, "s390-virtio") &&
+        virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_S390) && def->memballoon)
+        def->memballoon->model = VIR_DOMAIN_MEMBALLOON_MODEL_NONE;
+
     if (addDefaultUSBKBD &&
         def->ngraphics > 0 &&
         virDomainDefMaybeAddInput(def,
@@ -5980,6 +5984,33 @@  qemuDomainDeviceDefValidateInput(const virDomainInputDef *input,
 }
 
 
+static int
+qemuDomainDeviceDefValidateMemballoon(const virDomainMemballoonDef *memballoon,
+                                      virQEMUCapsPtr qemuCaps)
+{
+    if (!memballoon ||
+        memballoon->model == VIR_DOMAIN_MEMBALLOON_MODEL_NONE) {
+        return 0;
+    }
+
+    if (memballoon->model != VIR_DOMAIN_MEMBALLOON_MODEL_VIRTIO) {
+        virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+                       _("Memory balloon device type '%s' is not supported by this version of qemu"),
+                       virDomainMemballoonModelTypeToString(memballoon->model));
+        return -1;
+    }
+
+    if (memballoon->autodeflate != VIR_TRISTATE_SWITCH_ABSENT &&
+        !virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_BALLOON_AUTODEFLATE)) {
+        virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+                       _("deflate-on-oom is not supported by this QEMU binary"));
+        return -1;
+    }
+
+    return 0;
+}
+
+
 static int
 qemuDomainDeviceDefValidateZPCIAddress(virDomainDeviceInfoPtr info,
                                        virQEMUCapsPtr qemuCaps)
@@ -6088,11 +6119,14 @@  qemuDomainDeviceDefValidate(const virDomainDeviceDef *dev,
         ret = qemuDomainDeviceDefValidateInput(dev->data.input, def, qemuCaps);
         break;
 
+    case VIR_DOMAIN_DEVICE_MEMBALLOON:
+        ret = qemuDomainDeviceDefValidateMemballoon(dev->data.memballoon, qemuCaps);
+        break;
+
     case VIR_DOMAIN_DEVICE_LEASE:
     case VIR_DOMAIN_DEVICE_FS:
     case VIR_DOMAIN_DEVICE_SOUND:
     case VIR_DOMAIN_DEVICE_HUB:
-    case VIR_DOMAIN_DEVICE_MEMBALLOON:
     case VIR_DOMAIN_DEVICE_NVRAM:
     case VIR_DOMAIN_DEVICE_SHMEM:
     case VIR_DOMAIN_DEVICE_MEMORY:
diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c
index cb1e5f9e40..1802c36b86 100644
--- a/src/qemu/qemu_domain_address.c
+++ b/src/qemu/qemu_domain_address.c
@@ -358,8 +358,8 @@  qemuDomainPrimeVirtioDeviceAddresses(virDomainDefPtr def,
             def->hostdevs[i]->info->type = type;
     }
 
-    if (def->memballoon &&
-        def->memballoon->model == VIR_DOMAIN_MEMBALLOON_MODEL_VIRTIO &&
+    /* All memballoon devices accepted by the qemu driver are virtio */
+    if (virDomainDefHasMemballoon(def) &&
         def->memballoon->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE)
         def->memballoon->info.type = type;
 
@@ -2268,11 +2268,9 @@  qemuDomainAssignDevicePCISlots(virDomainDefPtr def,
             goto error;
     }
 
-    /* VirtIO balloon */
-    if (def->memballoon &&
-        def->memballoon->model == VIR_DOMAIN_MEMBALLOON_MODEL_VIRTIO &&
+    /* memballoon. the qemu driver only accepts virtio memballoon devices */
+    if (virDomainDefHasMemballoon(def) &&
         virDeviceInfoPCIAddressIsWanted(&def->memballoon->info)) {
-
         if (qemuDomainPCIAddressReserveNextAddr(addrs,
                                                 &def->memballoon->info) < 0)
             goto error;
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 90319261ff..26a83a754b 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -2439,11 +2439,10 @@  static int qemuDomainSetMemoryStatsPeriod(virDomainPtr dom, int period,
     priv = vm->privateData;
 
     if (def) {
-        if (!def->memballoon ||
-            def->memballoon->model != VIR_DOMAIN_MEMBALLOON_MODEL_VIRTIO) {
+        if (!virDomainDefHasMemballoon(def)) {
             virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
-                           _("Memory balloon model must be virtio to set the"
-                             " collection period"));
+                           _("No memory balloon device configured, "
+                             "can not set the collection period"));
             goto endjob;
         }
 
@@ -2463,11 +2462,10 @@  static int qemuDomainSetMemoryStatsPeriod(virDomainPtr dom, int period,
     }
 
     if (persistentDef) {
-        if (!persistentDef->memballoon ||
-            persistentDef->memballoon->model != VIR_DOMAIN_MEMBALLOON_MODEL_VIRTIO) {
+        if (!virDomainDefHasMemballoon(def)) {
             virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
-                           _("Memory balloon model must be virtio to set the"
-                             " collection period"));
+                           _("No memory balloon device configured, "
+                             "can not set the collection period"));
             goto endjob;
         }
         persistentDef->memballoon->period = period;
@@ -11964,8 +11962,7 @@  qemuDomainMemoryStatsInternal(virQEMUDriverPtr driver,
     if (virDomainObjCheckActive(vm) < 0)
         return -1;
 
-    if (vm->def->memballoon &&
-        vm->def->memballoon->model == VIR_DOMAIN_MEMBALLOON_MODEL_VIRTIO) {
+    if (virDomainDefHasMemballoon(vm->def)) {
         qemuDomainObjEnterMonitor(driver, vm);
         ret = qemuMonitorGetMemoryStats(qemuDomainGetMonitor(vm),
                                         vm->def->memballoon, stats, nr_stats);
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 8120201eb6..9ccc3601a2 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -7593,8 +7593,7 @@  int qemuProcessAttach(virConnectPtr conn ATTRIBUTE_UNUSED,
     if (running) {
         virDomainObjSetState(vm, VIR_DOMAIN_RUNNING,
                              VIR_DOMAIN_RUNNING_UNPAUSED);
-        if (vm->def->memballoon &&
-            vm->def->memballoon->model == VIR_DOMAIN_MEMBALLOON_MODEL_VIRTIO &&
+        if (virDomainDefHasMemballoon(vm->def) &&
             vm->def->memballoon->period) {
             qemuDomainObjEnterMonitor(driver, vm);
             qemuMonitorSetMemoryStatsPeriod(priv->mon, vm->def->memballoon,