| Message ID | 20231102114054.44360-39-quintela@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [PULL,01/40] hw/ipmi: Don't call vmstate_register() from instance_init() functions | expand |
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
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
diff --git a/migration/migration.c b/migration/migration.c index 70725a89ab..10a6f2fb12 100644 --- a/migration/migration.c +++ b/migration/migration.c @@ -458,9 +458,10 @@ void migrate_add_address(SocketAddress *address) } static bool migrate_uri_parse(const char *uri, - MigrationAddress **channel, + MigrationChannel **channel, Error **errp) { + g_autoptr(MigrationChannel) val = g_new0(MigrationChannel, 1); g_autoptr(MigrationAddress) addr = g_new0(MigrationAddress, 1); SocketAddress *saddr = NULL; InetSocketAddress *isock = &addr->u.rdma; @@ -505,7 +506,9 @@ static bool migrate_uri_parse(const char *uri, return false; } - *channel = g_steal_pointer(&addr); + val->channel_type = MIGRATION_CHANNEL_TYPE_MAIN; + val->addr = g_steal_pointer(&addr); + *channel = g_steal_pointer(&val); return true; } @@ -513,44 +516,47 @@ static void qemu_start_incoming_migration(const char *uri, bool has_channels, MigrationChannelList *channels, Error **errp) { - g_autoptr(MigrationAddress) channel = NULL; + MigrationChannel *channel = NULL; + MigrationAddress *addr = NULL; MigrationIncomingState *mis = migration_incoming_get_current(); /* * 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-incoming' 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; + } + } else { error_setg(errp, "neither 'uri' or 'channels' argument are " "specified in 'migrate-incoming' qmp command "); return; } - - if (uri && !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; } migrate_set_state(&mis->state, MIGRATION_STATUS_NONE, MIGRATION_STATUS_SETUP); - 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) { @@ -559,7 +565,7 @@ static void qemu_start_incoming_migration(const char *uri, bool has_channels, fd_start_incoming_migration(saddr->u.fd.str, errp); } #ifdef CONFIG_RDMA - } else if (channel->transport == MIGRATION_ADDRESS_TYPE_RDMA) { + } else if (addr->transport == MIGRATION_ADDRESS_TYPE_RDMA) { if (migrate_compress()) { error_setg(errp, "RDMA and compression can't be used together"); return; @@ -572,12 +578,12 @@ static void qemu_start_incoming_migration(const char *uri, bool has_channels, error_setg(errp, "RDMA and multifd can't be used together"); return; } - rdma_start_incoming_migration(&channel->u.rdma, errp); + rdma_start_incoming_migration(&addr->u.rdma, errp); #endif - } else if (channel->transport == MIGRATION_ADDRESS_TYPE_EXEC) { - exec_start_incoming_migration(channel->u.exec.args, errp); - } else if (channel->transport == MIGRATION_ADDRESS_TYPE_FILE) { - file_start_incoming_migration(&channel->u.file, errp); + } else if (addr->transport == MIGRATION_ADDRESS_TYPE_EXEC) { + exec_start_incoming_migration(addr->u.exec.args, errp); + } else if (addr->transport == MIGRATION_ADDRESS_TYPE_FILE) { + file_start_incoming_migration(&addr->u.file, errp); } else { error_setg(errp, "unknown migration protocol: %s", uri); } @@ -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; /* * 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; + } + } 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");