Re: [PULL 38/40] migration: Implement MigrateChannelList to qmp migration flow.

[Peter Maydell]

On Thu, 2 Nov 2023 at 11:46, Juan Quintela <quintela@redhat.com> wrote:
>
> From: Het Gala <het.gala@nutanix.com>
>
> Integrate MigrateChannelList with all transport backends
> (socket, exec and rdma) for both src and dest migration
> endpoints for qmp migration.
>
> For current series, limit the size of MigrateChannelList
> to single element (single interface) as runtime check.
>
> Suggested-by: Aravind Retnakaran <aravind.retnakaran@nutanix.com>
> Signed-off-by: Het Gala <het.gala@nutanix.com>
> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> Reviewed-by: Juan Quintela <quintela@redhat.com>
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> Message-ID: <20231023182053.8711-13-farosas@suse.de>

Hi; after this migration pull there seem to be a lot of
new Coverity issues in migration code. Could somebody
take a look at them, please?

Here's one in particular (CID 1523826, 1523828):

> @@ -1927,35 +1933,38 @@ void qmp_migrate(const char *uri, bool has_channels,
>      bool resume_requested;
>      Error *local_err = NULL;
>      MigrationState *s = migrate_get_current();
> -    g_autoptr(MigrationAddress) channel = NULL;
> +    MigrationChannel *channel = NULL;
> +    MigrationAddress *addr = NULL;

'channel' in this function used to be auto-freed, but now it is not...

>
>      /*
>       * Having preliminary checks for uri and channel
>       */
> -    if (has_channels) {
> -        error_setg(errp, "'channels' argument should not be set yet.");
> -        return;
> -    }
> -
>      if (uri && has_channels) {
>          error_setg(errp, "'uri' and 'channels' arguments are mutually "
>                     "exclusive; exactly one of the two should be present in "
>                     "'migrate' qmp command ");
>          return;
> -    }
> -
> -    if (!uri && !has_channels) {
> +    } else if (channels) {
> +        /* To verify that Migrate channel list has only item */
> +        if (channels->next) {
> +            error_setg(errp, "Channel list has more than one entries");
> +            return;
> +        }
> +        channel = channels->value;
> +    } else if (uri) {
> +        /* caller uses the old URI syntax */
> +        if (!migrate_uri_parse(uri, &channel, errp)) {
> +            return;
> +        }

...and here migrate_uri_parse() allocates memory which is
returned into 'channel'...

> +    } else {
>          error_setg(errp, "neither 'uri' or 'channels' argument are "
>                     "specified in 'migrate' qmp command ");
>          return;
>      }
> -
> -    if (!migrate_uri_parse(uri, &channel, errp)) {
> -        return;
> -    }
> +    addr = channel->addr;
>
>      /* transport mechanism not suitable for migration? */
> -    if (!migration_channels_and_transport_compatible(channel, errp)) {
> +    if (!migration_channels_and_transport_compatible(addr, errp)) {
>          return;
>      }
>
> @@ -1972,8 +1981,8 @@ void qmp_migrate(const char *uri, bool has_channels,
>          }
>      }
>
> -    if (channel->transport == MIGRATION_ADDRESS_TYPE_SOCKET) {
> -        SocketAddress *saddr = &channel->u.socket;
> +    if (addr->transport == MIGRATION_ADDRESS_TYPE_SOCKET) {
> +        SocketAddress *saddr = &addr->u.socket;
>          if (saddr->type == SOCKET_ADDRESS_TYPE_INET ||
>              saddr->type == SOCKET_ADDRESS_TYPE_UNIX ||
>              saddr->type == SOCKET_ADDRESS_TYPE_VSOCK) {
> @@ -1982,13 +1991,13 @@ void qmp_migrate(const char *uri, bool has_channels,
>              fd_start_outgoing_migration(s, saddr->u.fd.str, &local_err);
>          }
>  #ifdef CONFIG_RDMA
> -    } else if (channel->transport == MIGRATION_ADDRESS_TYPE_RDMA) {
> -        rdma_start_outgoing_migration(s, &channel->u.rdma, &local_err);
> +    } else if (addr->transport == MIGRATION_ADDRESS_TYPE_RDMA) {
> +        rdma_start_outgoing_migration(s, &addr->u.rdma, &local_err);
>  #endif
> -    } else if (channel->transport == MIGRATION_ADDRESS_TYPE_EXEC) {
> -        exec_start_outgoing_migration(s, channel->u.exec.args, &local_err);
> -    } else if (channel->transport == MIGRATION_ADDRESS_TYPE_FILE) {
> -        file_start_outgoing_migration(s, &channel->u.file, &local_err);
> +    } else if (addr->transport == MIGRATION_ADDRESS_TYPE_EXEC) {
> +        exec_start_outgoing_migration(s, addr->u.exec.args, &local_err);
> +    } else if (addr->transport == MIGRATION_ADDRESS_TYPE_FILE) {
> +        file_start_outgoing_migration(s, &addr->u.file, &local_err);
>      } else {
>          error_setg(&local_err, QERR_INVALID_PARAMETER_VALUE, "uri",
>                     "a valid migration protocol");

...which is leaked because we don't add any code for freeing
channel to compensate for it no longer being autofreed.

thanks
-- PMM