Re: [PATCH v4 4/4] tests/qtest/migration: add postcopy tests with multifd

[Prasad Pandit]

Hello Fabiano,

On Tue, 28 Jan 2025 at 02:43, Fabiano Rosas <farosas@suse.de> wrote:
> > +    if (args->multifd) {
> > +        migrate_set_capability(from, "multifd", true);
> > +        migrate_set_capability(to, "multifd", true);
>
> This is slightly backwards because currently that's what the hooks are
> for. I don't see the need for separate flags for multifd and
> postcopy. This also makes the code less maintainable because it creates
> two different ways of doing the same thing (hooks vs. args).

* I did look at the hook functions. In 'postcopy-tests.c' hook
function is not used. Fields are set in the 'MigrateCommon args'
object, which gets passed to migrate_postcopy_prepare() to
enable/disable capability.

* If we look at precopy-tests.c/tls-tests.c/compression-tests.c, the
same hook function 'migrate_hook_start_precopy_tcp_multifd_common'
gets called from them. Setting a capability therein shall affect all
tests which call that function. Defining a new hook function to set a
single field/capability and then calling the existing common hook
function for other attributes is doable, but doing so for multiple
qtests would only increase the number of hook functions, creating
clutter and confusion over time. (thinking aloud)

> Alternatively, we could add a more generic args->caps and have every test set 
> the capabilities it wants and
> the _common code to iterate over those and set them to true. Something
> like this perhaps:
>
>     for (int i = 0; i < MIGRATION_CAPABILITY__MAX; i++) {
>         if (args->caps[i]) {
>             migrate_set_capability(from, 
> MigrationCapability_str(args->caps[i]), true);
>             migrate_set_capability(to, 
> MigrationCapability_str(args->caps[i]), true);
>         }
>     }
>
> We could also set the number of channels as a default value. The tests
> could overwrite it from the hook if needed.

* Yes, this seems like a better option, I'll give it a try.  But
should we include it in this patch series OR make it a separate one?
I'm leaning towards the latter, because it is a generic change
affecting all tests, it's not specific to this series.

Thank you.
---
  - Prasad