Re: [Qemu-devel] [PATCH 5/7] balloon: Separate out stat and balloon handling

[Markus Armbruster]

Amit Shah <address@hidden> writes:

> Passing on '0' as ballooning target to indicate retrieval of stats is
> bad API.  It also makes 'balloon 0' in the monitor cause a segfault.
> Have two different functions handle the different functionality instead.
>
> Reported-by: Mike Cao <address@hidden>
> Signed-off-by: Amit Shah <address@hidden>

Can you explain the fault?  It's not obvious to me...

> ---
>  balloon.c           |   17 ++++++++++-------
>  balloon.h           |    8 +++++---
>  hw/virtio-balloon.c |    7 ++-----
>  3 files changed, 17 insertions(+), 15 deletions(-)
>
> diff --git a/balloon.c b/balloon.c
> index d40be39..8be3812 100644
> --- a/balloon.c
> +++ b/balloon.c
> @@ -32,30 +32,33 @@
>  
>  
>  static QEMUBalloonEvent *balloon_event_fn;
> +static QEMUBalloonStatus *balloon_stat_fn;
>  static void *balloon_opaque;
>  
> -void qemu_add_balloon_handler(QEMUBalloonEvent *func, void *opaque)
> +void qemu_add_balloon_handler(QEMUBalloonEvent *event_func,
> +                              QEMUBalloonStatus *stat_func, void *opaque)
>  {
> -    balloon_event_fn = func;
> +    balloon_event_fn = event_func;
> +    balloon_stat_fn = stat_func;
>      balloon_opaque = opaque;
>  }
>  
> -static int qemu_balloon(ram_addr_t target, MonitorCompletion cb, void 
> *opaque)
> +static int qemu_balloon(ram_addr_t target)
>  {
>      if (!balloon_event_fn) {
>          return 0;
>      }
>      trace_balloon_event(balloon_opaque, target);
> -    balloon_event_fn(balloon_opaque, target, cb, opaque);
> +    balloon_event_fn(balloon_opaque, target);
>      return 1;
>  }
>  
>  static int qemu_balloon_status(MonitorCompletion cb, void *opaque)
>  {
> -    if (!balloon_event_fn) {
> +    if (!balloon_stat_fn) {
>          return 0;
>      }
> -    balloon_event_fn(balloon_opaque, 0, cb, opaque);
> +    balloon_stat_fn(balloon_opaque, cb, opaque);
>      return 1;
>  }
>  
> @@ -135,7 +138,7 @@ int do_balloon(Monitor *mon, const QDict *params,
>          return -1;
>      }
>  
> -    ret = qemu_balloon(qdict_get_int(params, "value"), cb, opaque);
> +    ret = qemu_balloon(qdict_get_int(params, "value"));
>      if (ret == 0) {
>          qerror_report(QERR_DEVICE_NOT_ACTIVE, "balloon");
>          return -1;
> diff --git a/balloon.h b/balloon.h
> index 06a8a46..a6c31d5 100644
> --- a/balloon.h
> +++ b/balloon.h
> @@ -16,10 +16,12 @@
>  
>  #include "monitor.h"
>  
> -typedef void (QEMUBalloonEvent)(void *opaque, ram_addr_t target,
> -                                MonitorCompletion cb, void *cb_data);
> +typedef void (QEMUBalloonEvent)(void *opaque, ram_addr_t target);
> +typedef void (QEMUBalloonStatus)(void *opaque, MonitorCompletion cb,
> +                                 void *cb_data);
>  
> -void qemu_add_balloon_handler(QEMUBalloonEvent *func, void *opaque);
> +void qemu_add_balloon_handler(QEMUBalloonEvent *event_func,
> +                              QEMUBalloonStatus *stat_func, void *opaque);
>  
>  void monitor_print_balloon(Monitor *mon, const QObject *data);
>  int do_info_balloon(Monitor *mon, MonitorCompletion cb, void *opaque);
> diff --git a/hw/virtio-balloon.c b/hw/virtio-balloon.c
> index 2f371f2..40b43b0 100644
> --- a/hw/virtio-balloon.c
> +++ b/hw/virtio-balloon.c
> @@ -227,8 +227,7 @@ static void virtio_balloon_stat(void *opaque, 
> MonitorCompletion cb,
>      complete_stats_request(dev);
>  }
>  
> -static void virtio_balloon_to_target(void *opaque, ram_addr_t target,
> -                                     MonitorCompletion cb, void *cb_data)
> +static void virtio_balloon_to_target(void *opaque, ram_addr_t target)
>  {
>      VirtIOBalloon *dev = opaque;
>  
> @@ -238,8 +237,6 @@ static void virtio_balloon_to_target(void *opaque, 
> ram_addr_t target,
>      if (target) {
>          dev->num_pages = (ram_size - target) >> VIRTIO_BALLOON_PFN_SHIFT;
>          virtio_notify_config(&dev->vdev);
> -    } else {
> -        virtio_balloon_stat(opaque, cb, cb_data);
>      }
>  }
>  

Special case: do nothing when target == 0.  Is that necessary/

> @@ -284,7 +281,7 @@ VirtIODevice *virtio_balloon_init(DeviceState *dev)
>      s->svq = virtio_add_queue(&s->vdev, 128, virtio_balloon_receive_stats);
>  
>      reset_stats(s);
> -    qemu_add_balloon_handler(virtio_balloon_to_target, s);
> +    qemu_add_balloon_handler(virtio_balloon_to_target, virtio_balloon_stat, 
> s);
>  
>      register_savevm(dev, "virtio-balloon", -1, 1,
>                      virtio_balloon_save, virtio_balloon_load, s);