diff mbox series

[11/18] qemu: Move memballoon validation out of command.c

Message ID de2d1b33d59fba0c60ac613f6c01ec6173dd4241.1547746867.git.crobinso@redhat.com
State Superseded
Headers show
Series qemu: virtio-{non-}transitional support | expand

Commit Message

Cole Robinson Jan. 17, 2019, 5:52 p.m. UTC 
If we validate that memballoon is NONE or VIRTIO earlier,
we can simplify some checks in some driver APIs

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

---
 src/qemu/qemu_command.c        | 17 -----------------
 src/qemu/qemu_domain.c         | 35 +++++++++++++++++++++++++++++++++-
 src/qemu/qemu_domain_address.c |  3 +--
 src/qemu/qemu_driver.c         |  9 +++------
 src/qemu/qemu_process.c        |  3 +--
 5 files changed, 39 insertions(+), 28 deletions(-)

-- 
2.20.1

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

Comments

Andrea Bolognani Jan. 21, 2019, 1:55 p.m. UTC | #1 
On Thu, 2019-01-17 at 12:52 -0500, Cole Robinson wrote:
> If we validate that memballoon is NONE or VIRTIO earlier,
> we can simplify some checks in some driver APIs

Moving checks from the command line generation step to the domain
validation step - that's what I'm talking about! \o/

[...]
> +    if (STRPREFIX(def->os.machine, "s390-virtio") &&
> +        virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_S390) && def->memballoon)
> +        def->memballoon->model = VIR_DOMAIN_MEMBALLOON_MODEL_NONE;

This hunk of code makes zero sense to me, but that's what it's
looked like until now and nobody cares about s390-virtio anyway, so
I guess it doesn't matter ¯\_(ツ)_/¯

[...]
> +static int
> +qemuDomainDeviceDefValidateMemballoon(const virDomainMemballoonDef *memballoon,
> +                                      virQEMUCapsPtr qemuCaps)
> +{
> +    if (!memballoon ||
> +        memballoon->model == VIR_DOMAIN_MEMBALLOON_MODEL_NONE)
> +        return 0;

This needs curly braces around the body, as per our coding style.

[...]
> @@ -360,8 +360,7 @@ qemuDomainPrimeVirtioDeviceAddresses(virDomainDefPtr def,
>              def->hostdevs[i]->info->type = type;
>      }
>  
> -    if (def->memballoon &&
> -        def->memballoon->model == VIR_DOMAIN_MEMBALLOON_MODEL_VIRTIO &&
> +    if (virDomainDefHasMemballoon(def) &&
>          def->memballoon->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE)
>          def->memballoon->info.type = type;

Again, I don't think you can get away with removing the model check
here. As unlikely it is that we're ever going to get non-VirtIO
memory balloons down the line, this new code doesn't look quite
right to me, especially...

[...]
> @@ -2436,8 +2436,7 @@ 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"));

... if you look at the corresponding error messages.

I definitely like the change from checking def->memballoon to
calling virDomainDefHasMemballoon(def), though.
Cole Robinson Jan. 22, 2019, 5:29 p.m. UTC | #2 
On 01/21/2019 08:55 AM, Andrea Bolognani wrote:
> On Thu, 2019-01-17 at 12:52 -0500, Cole Robinson wrote:
>> If we validate that memballoon is NONE or VIRTIO earlier,
>> we can simplify some checks in some driver APIs
> 
> Moving checks from the command line generation step to the domain
> validation step - that's what I'm talking about! \o/
> 
> [...]
>> +    if (STRPREFIX(def->os.machine, "s390-virtio") &&
>> +        virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_S390) && def->memballoon)
>> +        def->memballoon->model = VIR_DOMAIN_MEMBALLOON_MODEL_NONE;
> 
> This hunk of code makes zero sense to me, but that's what it's
> looked like until now and nobody cares about s390-virtio anyway, so
> I guess it doesn't matter ¯\_(ツ)_/¯
> 

Yeah it's a strange one for sure...

> [...]
>> +static int
>> +qemuDomainDeviceDefValidateMemballoon(const virDomainMemballoonDef *memballoon,
>> +                                      virQEMUCapsPtr qemuCaps)
>> +{
>> +    if (!memballoon ||
>> +        memballoon->model == VIR_DOMAIN_MEMBALLOON_MODEL_NONE)
>> +        return 0;
> 
> This needs curly braces around the body, as per our coding style.
> 

Will do

> [...]
>> @@ -360,8 +360,7 @@ qemuDomainPrimeVirtioDeviceAddresses(virDomainDefPtr def,
>>              def->hostdevs[i]->info->type = type;
>>      }
>>  
>> -    if (def->memballoon &&
>> -        def->memballoon->model == VIR_DOMAIN_MEMBALLOON_MODEL_VIRTIO &&
>> +    if (virDomainDefHasMemballoon(def) &&
>>          def->memballoon->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE)
>>          def->memballoon->info.type = type;
> 
> Again, I don't think you can get away with removing the model check
> here. As unlikely it is that we're ever going to get non-VirtIO
> memory balloons down the line, this new code doesn't look quite
> right to me, especially...
> 
> [...]
>> @@ -2436,8 +2436,7 @@ 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"));
> 
> ... if you look at the corresponding error messages.
> 
> I definitely like the change from checking def->memballoon to
> calling virDomainDefHasMemballoon(def), though.
> 

This discussion is a follow on from the previous one about rng... in
this case I don't think it's worth extending the VIRTIO whitelist check
in these two additional places, compared to just covering this at
Validate time. In this StatsPeriod case, I can change this error message to

"No memory balloon device configured, can not set the collection period"

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 56d52e1d7e..07fa2b9209 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -4160,20 +4160,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;
@@ -4184,12 +4173,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 08bb2f9ebc..d2c792e415 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -3404,6 +3404,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,
@@ -5888,6 +5892,32 @@  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)
@@ -5996,11 +6026,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 b7a1cbe2d1..09e0ce12c4 100644
--- a/src/qemu/qemu_domain_address.c
+++ b/src/qemu/qemu_domain_address.c
@@ -360,8 +360,7 @@  qemuDomainPrimeVirtioDeviceAddresses(virDomainDefPtr def,
             def->hostdevs[i]->info->type = type;
     }
 
-    if (def->memballoon &&
-        def->memballoon->model == VIR_DOMAIN_MEMBALLOON_MODEL_VIRTIO &&
+    if (virDomainDefHasMemballoon(def) &&
         def->memballoon->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE)
         def->memballoon->info.type = type;
 
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 1d961707cc..949c09aba4 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -2436,8 +2436,7 @@  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"));
@@ -2460,8 +2459,7 @@  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"));
@@ -11947,8 +11945,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 aad6c12552..54abd9170a 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -7564,8 +7564,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,