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(-)
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,
--
2.20.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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.
--
Andrea Bolognani / Red Hat / Virtualization
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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
© 2016 - 2026 Red Hat, Inc.