[libvirt] [PATCH 5/8] qemu: domain: Add helper to determine presence of memory baloon

[John Ferlan]

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,
>