On Thu, 2 Nov 2023 at 11:46, Juan Quintela <[email protected]> wrote:
>
> From: Het Gala <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Het Gala <[email protected]>
> Signed-off-by: Fabiano Rosas <[email protected]>
> Reviewed-by: Juan Quintela <[email protected]>
> Signed-off-by: Juan Quintela <[email protected]>
> Message-ID: <[email protected]>
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
