Re: [PATCH v2 7/9] tests/qtest/migration: Define a machine for all architectures

[Daniel P . Berrangé]

On Wed, Oct 11, 2023 at 04:28:41PM +0200, Juan Quintela wrote:
> Fabiano Rosas <farosas@suse.de> wrote:
> > Stop relying on defaults and select a machine explicitly for every
> > architecture.
> >
> > This is a prerequisite for being able to select machine types for
> > migration using different QEMU binaries for source and destination.
> >
> > Signed-off-by: Fabiano Rosas <farosas@suse.de>
> > ---
> >  tests/qtest/migration-test.c | 11 ++++++++++-
> >  1 file changed, 10 insertions(+), 1 deletion(-)
> >
> > diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
> > index 46f1c275a2..7c10ac925b 100644
> > --- a/tests/qtest/migration-test.c
> > +++ b/tests/qtest/migration-test.c
> > @@ -746,6 +746,7 @@ static int test_migrate_start(QTestState **from, 
> > QTestState **to,
> >      const char *kvm_opts = NULL;
> >      const char *arch = qtest_get_arch();
> >      const char *memory_size;
> > +    const char *machine;
> >  
> >      if (args->use_shmem) {
> >          if (!g_file_test("/dev/shm", G_FILE_TEST_IS_DIR)) {
> > @@ -758,11 +759,13 @@ static int test_migrate_start(QTestState **from, 
> > QTestState **to,
> >      got_dst_resume = false;
> >      if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) {
> >          memory_size = "150M";
> > +        machine = "pc";
> 
> I would suggest:
> 
>       if (strcmp(arch, "i386")) {
>           machine = "pc";
>       } else {
>           machine = "q35";
>       }
> 
> New development is happening in q35, so I think this should be the more 
> tested.
> 
> > @@ -774,10 +777,12 @@ static int test_migrate_start(QTestState **from, 
> > QTestState **to,
> >                                        "'nvramrc=hex .\" _\" begin %x %x "
> >                                        "do i c@ 1 + i c! 1000 +loop .\" B\" 
> > 0 "
> >                                        "until'", end_address, 
> > start_address);
> > +        machine = "pseries";
> >          arch_opts = g_strdup("-nodefaults -machine vsmt=8");
> >      } else if (strcmp(arch, "aarch64") == 0) {
> >          memory_size = "150M";
> > -        arch_opts = g_strdup_printf("-machine virt,gic-version=max -cpu 
> > max "
> > +        machine = "virt";
> > +        arch_opts = g_strdup_printf("-machine gic-version=max -cpu max "
> 
> Does this double -machine command line works?

Why not just call the variable 'machine_opts' and here you can
do

 -        arch_opts = g_strdup_printf("-machine virt,gic-version=max -cpu max "
 +        machine_opts = "virt,gic-version=max";
 +        arch_opts = g_strdup_printf("-cpu max "


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|