On Wed, Oct 25, 2023 at 12:00:12PM -0300, Fabiano Rosas wrote: > Daniel P. Berrangé <berra...@redhat.com> writes: > > > On Wed, Oct 25, 2023 at 11:12:38AM -0300, Fabiano Rosas wrote: > >> Daniel P. Berrangé <berra...@redhat.com> writes: > >> > >> > On Mon, Oct 23, 2023 at 05:35:58PM -0300, Fabiano Rosas wrote: > >> >> Allow multifd to open file-backed channels. This will be used when > >> >> enabling the fixed-ram migration stream format which expects a > >> >> seekable transport. > >> >> > >> >> The QIOChannel read and write methods will use the preadv/pwritev > >> >> versions which don't update the file offset at each call so we can > >> >> reuse the fd without re-opening for every channel. > >> >> > >> >> Note that this is just setup code and multifd cannot yet make use of > >> >> the file channels. > >> >> > >> >> Signed-off-by: Fabiano Rosas <faro...@suse.de> > >> >> --- > >> >> migration/file.c | 64 +++++++++++++++++++++++++++++++++++++++++-- > >> >> migration/file.h | 10 +++++-- > >> >> migration/migration.c | 2 +- > >> >> migration/multifd.c | 14 ++++++++-- > >> >> migration/options.c | 7 +++++ > >> >> migration/options.h | 1 + > >> >> 6 files changed, 90 insertions(+), 8 deletions(-) > >> >> > >> >> diff --git a/migration/file.c b/migration/file.c > >> >> index cf5b1bf365..93b9b7bf5d 100644 > >> >> --- a/migration/file.c > >> >> +++ b/migration/file.c > >> >> @@ -17,6 +17,12 @@ > >> > > >> >> +void file_send_channel_create(QIOTaskFunc f, void *data) > >> >> +{ > >> >> + QIOChannelFile *ioc; > >> >> + QIOTask *task; > >> >> + Error *errp = NULL; > >> >> + > >> >> + ioc = qio_channel_file_new_path(outgoing_args.fname, > >> >> + outgoing_args.flags, > >> >> + outgoing_args.mode, &errp); > >> >> + if (!ioc) { > >> >> + file_migration_cancel(errp); > >> >> + return; > >> >> + } > >> >> + > >> >> + task = qio_task_new(OBJECT(ioc), f, (gpointer)data, NULL); > >> >> + qio_task_run_in_thread(task, qio_channel_file_connect_worker, > >> >> + (gpointer)data, NULL, NULL); > >> >> +} > >> >> + > >> >> void file_start_outgoing_migration(MigrationState *s, const char > >> >> *filespec, > >> >> Error **errp) > >> >> { > >> >> - g_autofree char *filename = g_strdup(filespec); > >> >> g_autoptr(QIOChannelFile) fioc = NULL; > >> >> + g_autofree char *filename = g_strdup(filespec); > >> >> uint64_t offset = 0; > >> >> QIOChannel *ioc; > >> >> + int flags = O_CREAT | O_TRUNC | O_WRONLY; > >> >> + mode_t mode = 0660; > >> >> > >> >> trace_migration_file_outgoing(filename); > >> >> > >> >> @@ -50,12 +105,15 @@ void file_start_outgoing_migration(MigrationState > >> >> *s, const char *filespec, > >> >> return; > >> >> } > >> >> > >> >> - fioc = qio_channel_file_new_path(filename, O_CREAT | O_WRONLY | > >> >> O_TRUNC, > >> >> - 0600, errp); > >> > >> By the way, we're experimenting with add-fd to flesh out the interface > >> with libvirt and I see that the flags here can conflict with the flags > >> set on the fd passed through `virsh --pass-fd ...` due to this at > >> monitor_fdset_dup_fd_add(): > >> > >> if ((flags & O_ACCMODE) == (mon_fd_flags & O_ACCMODE)) { > >> fd = mon_fdset_fd->fd; > >> break; > >> } > >> > >> We're requiring the O_RDONLY, O_WRONLY, O_RDWR flags defined here to > >> match the fdset passed into QEMU. Should we just sync the code of the > >> two projects to use the same flags? That feels a little clumsy to me. > > > > Is there a reason for libvirt to have set O_RDONLY for a file used > > for outgoing migration ? I can't recall off-hand. > > > > The flags need to match exactly, so either libvirt or QEMU could in the > future decide to use O_RDWR. Then we'd have a compatibility problem when > passing the fds around.
The "safe" option would be to always open O_RDWR, even if it is technically redundant for our current needs. 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 :|