Re: [Qemu-devel] [PATCH 07/12] migration: Allow for a limited number of announce timers

[Dr. David Alan Gilbert]

* Vladislav Yasevich (address@hidden) wrote:
> We currently create a new announcement timer every time
> qemu_announce_self() is called.  Since this is now a qmp
> command, this can lead to abuse.   Limit the number of
> timers that are created.  Give QMP interface and migration
> process 1 timer each.  This way, QMP can't abuse the
> announce_self mechanism.
> 
> Signed-off-by: Vladislav Yasevich <address@hidden>
> ---
>  include/migration/vmstate.h |  1 +
>  include/sysemu/sysemu.h     |  9 ++++++++-
>  migration/migration.c       |  2 +-
>  migration/savevm.c          | 24 +++++++++++++++++++-----
>  4 files changed, 29 insertions(+), 7 deletions(-)
> 
> diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
> index f8aed9b..689b685 100644
> --- a/include/migration/vmstate.h
> +++ b/include/migration/vmstate.h
> @@ -1057,6 +1057,7 @@ void vmstate_register_ram_global(struct MemoryRegion 
> *memory);
>  
>  typedef struct AnnounceTimer {
>      QEMUTimer *tm;
> +    struct AnnounceTimer **entry;
>      AnnounceParameters params;
>      QEMUClockType type;
>      int round;
> diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
> index 2ef1687..85a2af1 100644
> --- a/include/sysemu/sysemu.h
> +++ b/include/sysemu/sysemu.h
> @@ -78,14 +78,21 @@ void qemu_remove_machine_init_done_notifier(Notifier 
> *notify);
>  int save_vmstate(const char *name, Error **errp);
>  int load_vmstate(const char *name, Error **errp);
>  
> +typedef enum AnnounceType {
> +    QEMU_ANNOUNCE_MIGRATION,
> +    QEMU_ANNOUNCE_USER,
> +    QEMU_ANNOUNCE__MAX,
> +} AnnounceType;
> +
>  AnnounceParameters *qemu_get_announce_params(void);
>  void qemu_fill_announce_parameters(AnnounceParameters **to,
>                                     AnnounceParameters *from);
> +
>  bool qemu_validate_announce_parameters(AnnounceParameters *params,
>                                         Error **errp);
>  void qemu_set_announce_parameters(AnnounceParameters *announce_params,
>                                    AnnounceParameters *params);
> -void qemu_announce_self(AnnounceParameters *params);
> +void qemu_announce_self(AnnounceParameters *params, AnnounceType type);
>  
>  /* Subcommands for QEMU_VM_COMMAND */
>  enum qemu_vm_cmd {
> diff --git a/migration/migration.c b/migration/migration.c
> index 987c1cf..724fc40 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -345,7 +345,7 @@ static void process_incoming_migration_bh(void *opaque)
>       * This must happen after all error conditions are dealt with and
>       * we're sure the VM is going to be running on this host.
>       */
> -    qemu_announce_self(qemu_get_announce_params());
> +    qemu_announce_self(qemu_get_announce_params(), QEMU_ANNOUNCE_MIGRATION);
>  
>      /* If global state section was not received or we are in running
>         state, we need to obey autostart. Any other state is set with
> diff --git a/migration/savevm.c b/migration/savevm.c
> index b55ce6a..dcba8bd 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -218,6 +218,8 @@ static void qemu_announce_self_iter(NICState *nic, void 
> *opaque)
>      }
>  }
>  
> +AnnounceTimer *announce_timers[QEMU_ANNOUNCE__MAX];
> +
>  static void qemu_announce_self_once(void *opaque)
>  {
>      AnnounceTimer *timer = (AnnounceTimer *)opaque;
> @@ -228,6 +230,7 @@ static void qemu_announce_self_once(void *opaque)
>          timer_mod(timer->tm, qemu_clock_get_ms(timer->type) +
>                    self_announce_delay(timer));
>      } else {
> +            *(timer->entry) = NULL;
>              timer_del(timer->tm);
>              timer_free(timer->tm);
>              g_free(timer);
> @@ -256,12 +259,23 @@ AnnounceTimer 
> *qemu_announce_timer_create(AnnounceParameters *params,
>      return timer;
>  }
>  
> -void qemu_announce_self(AnnounceParameters *params)
> +void qemu_announce_self(AnnounceParameters *params, AnnounceType type)
>  {
>      AnnounceTimer *timer;
>  
> -    timer = qemu_announce_timer_create(params, QEMU_CLOCK_REALTIME,
> -                                       qemu_announce_self_once);
> +    timer = announce_timers[type];
> +    if (!timer) {
> +        timer = qemu_announce_timer_create(params, QEMU_CLOCK_REALTIME,
> +                                            qemu_announce_self_once);
> +        announce_timers[type] = timer;
> +        timer->entry = &announce_timers[type];
> +    } else {
> +        /* For now, don't do anything.  If we want to reset the timer,
> +         * we'll need to add locking to each announce timer to prevent
> +         * races between timeout handling and a reset.
> +         */

I worry that this is racy anyway; if you issue a command and it doesn't
start because it's still doing the last one and you don't get any
warning of that it's difficult (as in my comment on the 12th).

Is this really racy, isn't this in the big lock ? Hmm I guess the qmp
triggered one is, this probably isn't.

Dave

> +        return;
> +    }
>      qemu_announce_self_once(timer);
>  }
>  
> @@ -276,7 +290,7 @@ void qmp_announce_self(bool has_params, 
> AnnounceParameters *params,
>      if (has_params)
>          qemu_set_announce_parameters(&announce_params, params);
>  
> -    qemu_announce_self(&announce_params);
> +    qemu_announce_self(&announce_params, QEMU_ANNOUNCE_USER);
>  }
>  
>  /***********************************************************/
> @@ -1750,7 +1764,7 @@ static void loadvm_postcopy_handle_run_bh(void *opaque)
>       */
>      cpu_synchronize_all_post_init();
>  
> -    qemu_announce_self(qemu_get_announce_params());
> +    qemu_announce_self(qemu_get_announce_params(), QEMU_ANNOUNCE_MIGRATION);
>  
>      /* Make sure all file formats flush their mutable metadata.
>       * If we get an error here, just don't restart the VM yet. */
> -- 
> 2.7.4
> 
--
Dr. David Alan Gilbert / address@hidden / Manchester, UK