Re: [PATCH 2/4] tests/migration-test: Merge shmem_opts into memory_backend

[Peter Xu <peterx@redhat.com>]

On Fri, Nov 21, 2025 at 01:46:31PM +0100, Juraj Marcin wrote:
> Hi Peter,
> 
> you can add my R-b to the whole series, I just have a small nitpick
> below, but feel free to ignore it.
> 
> Reviewed-by: Juraj Marcin <jmarcin@redhat.com>

Thanks a lot for the quick look. :)

> 
> On 2025-11-17 17:39, Peter Xu wrote:
> > The two parameters are more or less duplicated in migrate_args().  They all
> > describe the memory type.  When one is used, the other is not.
> > 
> > mem_type currently uses numa parameter to specify the memory backend, while
> > memory_backend (the two users of such uses "-machine memory-backend=ID").
> > 
> > This patch merges the use of the two variables so that we always generate a
> > memory object string and put it into "memory_backend" variable.  Now we can
> > drop shmem_opts parameter in the function.
> > 
> > Meanwhile we always use a memory-backend-* no matter which mem type is
> > used.  This brings mem_type to be aligned with memory_backend usage, then
> > we stick with this as this is flexible enough.
> > 
> > This paves way that we merge mem_type and memory_backend in MigrateStart.
> > 
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >  tests/qtest/migration/framework.c | 41 ++++++++++++++++++++-----------
> >  1 file changed, 26 insertions(+), 15 deletions(-)
> > 
> > diff --git a/tests/qtest/migration/framework.c b/tests/qtest/migration/framework.c
> > index 8fa39999a1..6df0e56c2a 100644
> > --- a/tests/qtest/migration/framework.c
> > +++ b/tests/qtest/migration/framework.c
> > @@ -260,23 +260,36 @@ static char *test_shmem_path(void)
> >      return g_strdup_printf("/dev/shm/qemu-%d", getpid());
> >  }
> >  
> > +#define  MIG_MEM_ID  "mig.mem"
> > +
> >  /* NOTE: caller is responsbile to free the string if returned */
> >  static char *migrate_mem_type_get_opts(MemType type, const char *memory_size)
> >  {
> >      g_autofree char *shmem_path = NULL;
> > -    char *backend = NULL;
> > +    g_autofree char *backend = NULL;
> > +    bool share = true;
> > +    char *opts;
> >  
> >      switch (type) {
> >      case MEM_TYPE_SHMEM:
> >          shmem_path = test_shmem_path();
> > -        backend = g_strdup_printf(
> > -            "-object memory-backend-file,id=mem0,size=%s"
> > -            ",mem-path=%s,share=on -numa node,memdev=mem0",
> > -            memory_size, shmem_path);
> > -        return backend;
> > +        backend = g_strdup_printf("-object memory-backend-file,mem-path=%s",
> > +                                  shmem_path);
> > +        break;
> > +    case MEM_TYPE_ANON:
> > +        backend = g_strdup("-object memory-backend-ram");
> > +        share = false;
> > +        break;
> 
> Wouldn't it be a bit nicer to add this case before SHMEM, so the order
> is the same as in the enum? The patch adding MEMFD follows is by adding
> it right after SHMEM (and before ANON).

Normally we don't need to order cases in a switch() (especially think about
when fallthrough cases could happen..), but sure I am happy to move it if
that at least makes it easier to read for you.

I'll wait for other comments to see if I'll just move it when merge or
repost.

Thanks,

> 
> >      default:
> > -        return NULL;
> > +        g_assert_not_reached();
> > +        break;
> >      }
> > +
> > +    opts = g_strdup_printf("%s,id=%s,size=%s,share=%s",
> > +                           backend, MIG_MEM_ID, memory_size,
> > +                           share ? "on" : "off");
> > +
> > +    return opts;
> >  }
> >  
> >  int migrate_args(char **from, char **to, const char *uri, MigrateStart *args)
> > @@ -286,7 +299,7 @@ int migrate_args(char **from, char **to, const char *uri, MigrateStart *args)
> >      gchar *cmd_source = NULL;
> >      gchar *cmd_target = NULL;
> >      const gchar *ignore_stderr;
> > -    g_autofree char *shmem_opts = NULL;
> > +    g_autofree char *mem_object = NULL;
> >      const char *kvm_opts = NULL;
> >      const char *arch = qtest_get_arch();
> >      const char *memory_size;
> > @@ -350,12 +363,12 @@ int migrate_args(char **from, char **to, const char *uri, MigrateStart *args)
> >          ignore_stderr = "";
> >      }
> >  
> > -    shmem_opts = migrate_mem_type_get_opts(args->mem_type, memory_size);
> > -
> >      if (args->memory_backend) {
> >          memory_backend = g_strdup_printf(args->memory_backend, memory_size);
> >      } else {
> > -        memory_backend = g_strdup_printf("-m %s ", memory_size);
> > +        mem_object = migrate_mem_type_get_opts(args->mem_type, memory_size);
> > +        memory_backend = g_strdup_printf("-machine memory-backend=%s %s",
> > +                                         MIG_MEM_ID, mem_object);
> >      }
> >  
> >      if (args->use_dirty_ring) {
> > @@ -378,12 +391,11 @@ int migrate_args(char **from, char **to, const char *uri, MigrateStart *args)
> >                                   "-name source,debug-threads=on "
> >                                   "%s "
> >                                   "-serial file:%s/src_serial "
> > -                                 "%s %s %s %s",
> > +                                 "%s %s %s",
> >                                   kvm_opts ? kvm_opts : "",
> >                                   machine, machine_opts,
> >                                   memory_backend, tmpfs,
> >                                   arch_opts ? arch_opts : "",
> > -                                 shmem_opts ? shmem_opts : "",
> >                                   args->opts_source ? args->opts_source : "",
> >                                   ignore_stderr);
> >  
> > @@ -400,13 +412,12 @@ int migrate_args(char **from, char **to, const char *uri, MigrateStart *args)
> >                                   "%s "
> >                                   "-serial file:%s/dest_serial "
> >                                   "-incoming %s "
> > -                                 "%s %s %s %s %s",
> > +                                 "%s %s %s %s",
> >                                   kvm_opts ? kvm_opts : "",
> >                                   machine, machine_opts,
> >                                   memory_backend, tmpfs, uri,
> >                                   events,
> >                                   arch_opts ? arch_opts : "",
> > -                                 shmem_opts ? shmem_opts : "",
> >                                   args->opts_target ? args->opts_target : "",
> >                                   ignore_stderr);
> >  
> > -- 
> > 2.50.1
> > 
> 

-- 
Peter Xu