On 04/06/2016 12:02 PM, Peter Krempa wrote:
> ---
> src/qemu/qemu_command.c | 3 +--
> src/qemu/qemu_domain.c | 11 +++++++++--
> src/qemu/qemu_domain.h | 1 +
> src/qemu/qemu_driver.c | 12 ++++--------
> src/qemu/qemu_process.c | 9 +++------
> 5 files changed, 18 insertions(+), 18 deletions(-)
>
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index a2448bf..1518a53 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -3408,8 +3408,7 @@ qemuBuildMemballoonCommandLine(virCommandPtr cmd,
> virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_S390) && def->memballoon)
> def->memballoon->model = VIR_DOMAIN_MEMBALLOON_MODEL_NONE;
>
> - if (!def->memballoon ||
> - def->memballoon->model == VIR_DOMAIN_MEMBALLOON_MODEL_NONE)
> + if (!qemuDomainDefHasMemballoon(def))
> return 0;
>
> if (def->memballoon->model != VIR_DOMAIN_MEMBALLOON_MODEL_VIRTIO) {
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index f38b0f3..c13acd1 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -4218,6 +4218,14 @@ qemuDomainMachineHasBuiltinIDE(const virDomainDef *def)
> }
>
>
> +bool
> +qemuDomainDefHasMemballoon(const virDomainDef *def)
> +{
> + return def->memballoon &&
> + def->memballoon->model != VIR_DOMAIN_MEMBALLOON_MODEL_NONE;
I guess I would think would be if balloon and model == MODEL_VIRTIO
I find it easier to read absolutes rather than considering alternatives
to the != check... in this case that's only MODEL_XEN which yes should
never happen, but with the && at least there's no question...
> +}
> +
> +
> /**
> * qemuDomainUpdateCurrentMemorySize:
> *
> @@ -4242,8 +4250,7 @@ qemuDomainUpdateCurrentMemorySize(virQEMUDriverPtr driver,
>
> /* if no balloning is available, the current size equals to the current
> * full memory size */
> - if (!vm->def->memballoon ||
> - vm->def->memballoon->model == VIR_DOMAIN_MEMBALLOON_MODEL_NONE) {
> + if (!qemuDomainDefHasMemballoon(vm->def)) {
> vm->def->mem.cur_balloon = virDomainDefGetMemoryActual(vm->def);
> return 0;
> }
> diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
> index 54d7bd7..2992214 100644
> --- a/src/qemu/qemu_domain.h
> +++ b/src/qemu/qemu_domain.h
> @@ -516,6 +516,7 @@ bool qemuDomainHasBlockjob(virDomainObjPtr vm, bool copy_only)
> int qemuDomainAlignMemorySizes(virDomainDefPtr def);
> void qemuDomainMemoryDeviceAlignSize(virDomainDefPtr def,
> virDomainMemoryDefPtr mem);
> +bool qemuDomainDefHasMemballoon(const virDomainDef *def);
>
> virDomainChrDefPtr qemuFindAgentConfig(virDomainDefPtr def);
>
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index eaabe58..a00268b 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -2505,8 +2505,7 @@ static int qemuDomainSetMemoryStatsPeriod(virDomainPtr dom, int period,
> priv = vm->privateData;
>
> if (def) {
> - if (!def->memballoon ||
> - def->memballoon->model != VIR_DOMAIN_MEMBALLOON_MODEL_VIRTIO) {
This is what I was thinking - either this construct or the corollary
balloon && model == VIRTIO.
> + if (!qemuDomainDefHasMemballoon(def)) {
> virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> _("Memory balloon model must be virtio to set the"
> " collection period"));
> @@ -2529,8 +2528,7 @@ static int qemuDomainSetMemoryStatsPeriod(virDomainPtr dom, int period,
> }
>
> if (persistentDef) {
> - if (!persistentDef->memballoon ||
> - persistentDef->memballoon->model != VIR_DOMAIN_MEMBALLOON_MODEL_VIRTIO) {
> + if (!qemuDomainDefHasMemballoon(persistentDef)) {
> virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> _("Memory balloon model must be virtio to set the"
> " collection period"));
> @@ -11495,8 +11493,7 @@ qemuDomainMemoryStats(virDomainPtr dom,
> goto endjob;
> }
>
> - if (vm->def->memballoon &&
> - vm->def->memballoon->model == VIR_DOMAIN_MEMBALLOON_MODEL_VIRTIO) {
> + if (qemuDomainDefHasMemballoon(vm->def)) {
> priv = vm->privateData;
> qemuDomainObjEnterMonitor(driver, vm);
> ret = qemuMonitorGetMemoryStats(priv->mon, stats, nr_stats);
> @@ -19061,8 +19058,7 @@ qemuDomainGetStatsBalloon(virQEMUDriverPtr driver ATTRIBUTE_UNUSED,
> unsigned long long cur_balloon = 0;
> int err = 0;
>
> - if (dom->def->memballoon &&
> - dom->def->memballoon->model == VIR_DOMAIN_MEMBALLOON_MODEL_NONE) {
> + if (!qemuDomainDefHasMemballoon(dom->def)) {
Ugh.. the ugly duckling
This changes the statement from if we have a model=NONE memballoon to we
have a memballoon with a model that's not NONE. Double negatives.
I think the logic has the right thought, but would be perhaps clearer in
my mind with the adjusted logic in the common function.
I think this is ACK-able just want get your thoughts on the logic for
the helper...
John
> cur_balloon = virDomainDefGetMemoryActual(dom->def);
> } else if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BALLOON_EVENT)) {
> cur_balloon = dom->def->mem.cur_balloon;
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index d9dca74..4911c1d 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -1933,8 +1933,7 @@ qemuProcessRefreshBalloonState(virQEMUDriverPtr driver,
>
> /* if no ballooning is available, the current size equals to the current
> * full memory size */
> - if (!vm->def->memballoon ||
> - vm->def->memballoon->model == VIR_DOMAIN_MEMBALLOON_MODEL_NONE) {
> + if (!qemuDomainDefHasMemballoon(vm->def)) {
> vm->def->mem.cur_balloon = virDomainDefGetMemoryActual(vm->def);
> return 0;
> }
> @@ -4382,8 +4381,7 @@ qemuProcessSetupBalloon(virQEMUDriverPtr driver,
> int period;
> int ret = -1;
>
> - if (!vm->def->memballoon ||
> - vm->def->memballoon->model == VIR_DOMAIN_MEMBALLOON_MODEL_NONE)
> + if (!qemuDomainDefHasMemballoon(vm->def))
> return 0;
>
> period = vm->def->memballoon->period;
> @@ -6237,8 +6235,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 (qemuDomainDefHasMemballoon(vm->def) &&
> vm->def->memballoon->period) {
> qemuDomainObjEnterMonitor(driver, vm);
> qemuMonitorSetMemoryStatsPeriod(priv->mon,
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list