On Tue, Mar 12, 2024 at 08:22:18AM -0400, Peter Xu wrote: > On Tue, Mar 12, 2024 at 09:57:58AM +0000, Daniel P. Berrangé wrote: > > On Mon, Mar 11, 2024 at 08:33:35PM -0300, Fabiano Rosas wrote: > > > The file migration code was allowing a possible -1 from a failed call > > > to dup() to propagate into the new QIOFileChannel::fd before checking > > > for validity. Coverity doesn't like that, possibly due to the the > > > lseek(-1, ...) call that would ensue before returning from the channel > > > creation routine. > > > > > > Use the newly introduced qio_channel_file_dupfd() to properly check > > > the return of dup() before proceeding. > > > > > > Fixes: CID 1539961 > > > Fixes: CID 1539965 > > > Fixes: CID 1539960 > > > Fixes: 2dd7ee7a51 ("migration/multifd: Add incoming QIOChannelFile > > > support") > > > Fixes: decdc76772 ("migration/multifd: Add mapped-ram support to fd: URI") > > > Reported-by: Peter Maydell <peter.mayd...@linaro.org> > > > Signed-off-by: Fabiano Rosas <faro...@suse.de> > > > --- > > > migration/fd.c | 9 ++++----- > > > migration/file.c | 14 +++++++------- > > > 2 files changed, 11 insertions(+), 12 deletions(-) > > > > > > diff --git a/migration/fd.c b/migration/fd.c > > > index d4ae72d132..4e2a63a73d 100644 > > > --- a/migration/fd.c > > > +++ b/migration/fd.c > > > @@ -80,6 +80,7 @@ static gboolean fd_accept_incoming_migration(QIOChannel > > > *ioc, > > > void fd_start_incoming_migration(const char *fdname, Error **errp) > > > { > > > QIOChannel *ioc; > > > + QIOChannelFile *fioc; > > > int fd = monitor_fd_param(monitor_cur(), fdname, errp); > > > if (fd == -1) { > > > return; > > > @@ -103,15 +104,13 @@ void fd_start_incoming_migration(const char > > > *fdname, Error **errp) > > > int channels = migrate_multifd_channels(); > > > > > > while (channels--) { > > > - ioc = QIO_CHANNEL(qio_channel_file_new_fd(dup(fd))); > > > - > > > - if (QIO_CHANNEL_FILE(ioc)->fd == -1) { > > > - error_setg(errp, "Failed to duplicate fd %d", fd); > > > + fioc = qio_channel_file_new_dupfd(fd, errp); > > > + if (!fioc) { > > > return; > > > } > > > > > > qio_channel_set_name(ioc, "migration-fd-incoming"); > > > - qio_channel_add_watch_full(ioc, G_IO_IN, > > > + qio_channel_add_watch_full(QIO_CHANNEL(fioc), G_IO_IN, > > > fd_accept_incoming_migration, > > > NULL, NULL, > > > > > > g_main_context_get_thread_default()); > > > > Nothing is free'ing the already created channels, if this while() > > loop fails on the 2nd or later iterations. > > > > > diff --git a/migration/file.c b/migration/file.c > > > index 164b079966..d458f48269 100644 > > > --- a/migration/file.c > > > +++ b/migration/file.c > > > @@ -58,12 +58,13 @@ bool file_send_channel_create(gpointer opaque, Error > > > **errp) > > > int fd = fd_args_get_fd(); > > > > > > if (fd && fd != -1) { > > > - ioc = qio_channel_file_new_fd(dup(fd)); > > > + ioc = qio_channel_file_new_dupfd(fd, errp); > > > } else { > > > ioc = qio_channel_file_new_path(outgoing_args.fname, flags, 0, > > > errp); > > > - if (!ioc) { > > > - goto out; > > > - } > > > + } > > > + > > > + if (!ioc) { > > > + goto out; > > > } > > > > > > multifd_channel_connect(opaque, QIO_CHANNEL(ioc)); > > > @@ -147,10 +148,9 @@ void file_start_incoming_migration(FileMigrationArgs > > > *file_args, Error **errp) > > > NULL, NULL, > > > g_main_context_get_thread_default()); > > > > > > - fioc = qio_channel_file_new_fd(dup(fioc->fd)); > > > + fioc = qio_channel_file_new_dupfd(fioc->fd, errp); > > > > > > - if (!fioc || fioc->fd == -1) { > > > - error_setg(errp, "Error creating migration incoming > > > channel"); > > > + if (!fioc) { > > > break; > > > } > > > } while (++i < channels); > > > > Again, nothing is free'ing when the loops fails on 2nd or later > > iterations. > > For this one, I think it constantly leak one IOC even if no failure > triggers.. > > > > > So a weak > > > > Reviewed-by: Daniel P. Berrangé <berra...@redhat.com> > > > > on the basis that it fixes the bugs that it claims to fix, but there > > are more bugs that still need fixing here. > > For the other issue, Fabiano - I think there's one easy way to workaround > and avoid bothering with "how to remove a registered IO watch" is we create > the IOCs in a loop first, register the IO watches only if all succeeded.
Yes, that makes sense as an approach. With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|