Re: [Qemu-devel] [PATCH v2 2/6] migration: let MigrationState be a qdev

[Peter Xu]

On Fri, Jun 09, 2017 at 03:39:24PM +0200, Markus Armbruster wrote:
> Peter Xu <address@hidden> writes:
> 
> > Let the old man "MigrationState" join the object family. Direct benefit
> > is that we can start to use all the property features derived from
> > current QDev, like: HW_COMPAT_* bits, command line setup for migration
> > parameters (so will never need to set them up each time using HMP/QMP,
> > this is really, really attractive for test writters), etc.
> >
> > I see no reason to disallow this happen yet. So let's start from this
> > one, to see whether it would be anything good.
> >
> > No functional change at all.
> >
> > Signed-off-by: Peter Xu <address@hidden>
> > ---
> >  include/migration/migration.h | 19 ++++++++++++++
> >  migration/migration.c         | 61 
> > ++++++++++++++++++++++++++++---------------
> >  2 files changed, 59 insertions(+), 21 deletions(-)
> >
> > diff --git a/include/migration/migration.h b/include/migration/migration.h
> > index 79b5484..bd0186c 100644
> > --- a/include/migration/migration.h
> > +++ b/include/migration/migration.h
> > @@ -21,6 +21,7 @@
> >  #include "qapi-types.h"
> >  #include "exec/cpu-common.h"
> >  #include "qemu/coroutine_int.h"
> > +#include "hw/qdev.h"
> >  
> >  #define QEMU_VM_FILE_MAGIC           0x5145564d
> >  #define QEMU_VM_FILE_VERSION_COMPAT  0x00000002
> > @@ -49,6 +50,8 @@ enum mig_rp_message_type {
> >      MIG_RP_MSG_MAX
> >  };
> >  
> > +#define TYPE_MIGRATION "migration"
> > +
> >  /* State for the incoming migration */
> >  struct MigrationIncomingState {
> >      QEMUFile *from_src_file;
> > @@ -91,8 +94,24 @@ struct MigrationIncomingState {
> >  MigrationIncomingState *migration_incoming_get_current(void);
> >  void migration_incoming_state_destroy(void);
> >  
> > +#define MIGRATION_CLASS(klass) \
> > +    OBJECT_CLASS_CHECK(MigrationClass, (klass), TYPE_MIGRATION)
> > +#define MIGRATION_OBJ(obj) \
> > +    OBJECT_CHECK(MigrationState, (obj), TYPE_MIGRATION)
> > +#define MIGRATION_GET_CLASS(obj) \
> > +    OBJECT_GET_CLASS(MigrationClass, (obj), TYPE_MIGRATION)
> > +
> > +typedef struct MigrationClass {
> > +    /*< private >*/
> > +    DeviceClass parent_class;
> > +} MigrationClass;
> > +
> >  struct MigrationState
> >  {
> > +    /*< private >*/
> > +    DeviceState parent_obj;
> > +
> > +    /*< public >*/
> 
> Turning MigrationState into a QOM object so you can use QOM
> infrastructure makes sense.  But why turn it into a device?  It doesn't
> feel device-like to me.  Would ObjectClass and Object instead of
> DeviceClass and DeviceState do?

Yeah. That's the main reason for the series to be (was) RFC.

I was trying to use the HW_COMPAT_* bits and -global, and that's QDev
thing (IIUC you got that already :). I am just curious about why that
is not for QObject from the very beginning, then it'll be easier.

For now, IMHO QDev is okay as well for migration, as long as it's kept
internally inside QEMU. But sure at least I should turn user_creatable
to off. I'll investigate more to see how to make this a safer approach
in next post.

> 
> >      size_t bytes_xfer;
> >      size_t xfer_limit;
> >      QemuThread thread;
> > diff --git a/migration/migration.c b/migration/migration.c
> > index 48c94c9..98b77e2 100644
> > --- a/migration/migration.c
> > +++ b/migration/migration.c
> > @@ -93,29 +93,13 @@ static bool deferred_incoming;
> >  /* For outgoing */
> >  MigrationState *migrate_get_current(void)
> >  {
> > -    static bool once;
> > -    static MigrationState current_migration = {
> > -        .state = MIGRATION_STATUS_NONE,
> > -        .xbzrle_cache_size = DEFAULT_MIGRATE_CACHE_SIZE,
> > -        .mbps = -1,
> > -        .parameters = {
> > -            .compress_level = DEFAULT_MIGRATE_COMPRESS_LEVEL,
> > -            .compress_threads = DEFAULT_MIGRATE_COMPRESS_THREAD_COUNT,
> > -            .decompress_threads = DEFAULT_MIGRATE_DECOMPRESS_THREAD_COUNT,
> > -            .cpu_throttle_initial = DEFAULT_MIGRATE_CPU_THROTTLE_INITIAL,
> > -            .cpu_throttle_increment = 
> > DEFAULT_MIGRATE_CPU_THROTTLE_INCREMENT,
> > -            .max_bandwidth = MAX_THROTTLE,
> > -            .downtime_limit = DEFAULT_MIGRATE_SET_DOWNTIME,
> > -            .x_checkpoint_delay = DEFAULT_MIGRATE_X_CHECKPOINT_DELAY,
> > -        },
> > -    };
> > +    static MigrationState *current_migration;
> 
> You add an indirection...
> 
> >  
> > -    if (!once) {
> > -        current_migration.parameters.tls_creds = g_strdup("");
> > -        current_migration.parameters.tls_hostname = g_strdup("");
> > -        once = true;
> > +    if (!current_migration) {
> > +        current_migration = MIGRATION_OBJ(object_new(TYPE_MIGRATION));
> 
> ... possibly just so you can use object_new().  Have you considered
> object_initialize()?

Yes, but even with object_initialize() I still need that indirection?

Or, I can put the init into main() in some way to avoid the
indirection.

Thanks!

-- 
Peter Xu