On Thu, 2 Nov 2023 at 11:46, Juan Quintela <quint...@redhat.com> wrote: > > From: Het Gala <het.g...@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.retnaka...@nutanix.com> > Signed-off-by: Het Gala <het.g...@nutanix.com> > Signed-off-by: Fabiano Rosas <faro...@suse.de> > Reviewed-by: Juan Quintela <quint...@redhat.com> > Signed-off-by: Juan Quintela <quint...@redhat.com> > Message-ID: <20231023182053.8711-13-faro...@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