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.
