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

[Juan Quintela]

Peter Maydell <peter.maydell@linaro.org> wrote:
> 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?


Hi

I received this, looking into it.

Later, Juan.

>
> 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