Re: [PATCH V7 03/29] migration: qemu file wrappers

[Steven Sistare]

On 2/24/2022 1:21 PM, Dr. David Alan Gilbert wrote:
> * Steve Sistare (steven.sistare@oracle.com) wrote:
>> Add qemu_file_open and qemu_fd_open to create QEMUFile objects for unix
>> files and file descriptors.
>>
>> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
>> ---
>>  migration/qemu-file-channel.c | 36 ++++++++++++++++++++++++++++++++++++
>>  migration/qemu-file-channel.h |  6 ++++++
>>  2 files changed, 42 insertions(+)
>>
>> diff --git a/migration/qemu-file-channel.c b/migration/qemu-file-channel.c
>> index bb5a575..afb16d7 100644
>> --- a/migration/qemu-file-channel.c
>> +++ b/migration/qemu-file-channel.c
>> @@ -27,8 +27,10 @@
>>  #include "qemu-file.h"
>>  #include "io/channel-socket.h"
>>  #include "io/channel-tls.h"
>> +#include "io/channel-file.h"
>>  #include "qemu/iov.h"
>>  #include "qemu/yank.h"
>> +#include "qapi/error.h"
>>  #include "yank_functions.h"
>>  
>>  
>> @@ -192,3 +194,37 @@ QEMUFile *qemu_fopen_channel_output(QIOChannel *ioc)
>>      object_ref(OBJECT(ioc));
>>      return qemu_fopen_ops(ioc, &channel_output_ops, true);
>>  }
>> +
>> +QEMUFile *qemu_file_open(const char *path, int flags, int mode,
>> +                         const char *name, Error **errp)
> 
> Can you please make that qemu_fopen_file

Will do.

>> +{
>> +    g_autoptr(QIOChannelFile) fioc = NULL;
>> +    QIOChannel *ioc;
>> +    QEMUFile *f;
>> +
>> +    if (flags & O_RDWR) {
>> +        error_setg(errp, "qemu_file_open %s: O_RDWR not supported", path);
>> +        return NULL;
>> +    }
>> +
>> +    fioc = qio_channel_file_new_path(path, flags, mode, errp);
>> +    if (!fioc) {
>> +        return NULL;
>> +    }
>> +
>> +    ioc = QIO_CHANNEL(fioc);
>> +    qio_channel_set_name(ioc, name);
>> +    f = (flags & O_WRONLY) ? qemu_fopen_channel_output(ioc) :
>> +                             qemu_fopen_channel_input(ioc);
>> +    return f;
>> +}
>> +
>> +QEMUFile *qemu_fd_open(int fd, bool writable, const char *name)
>> +{
> 
> Can you please make that qemu_fopen_fd

Will do.

>>> +    g_autoptr(QIOChannelFile) fioc = qio_channel_file_new_fd(fd);
> 
> Can you use qio_channel_new_fd for that? Then it creates either
> a socket or file subclass depending what type of fd is passed
> (and gives you a QIOChannel without needing to cast).

The downside is that we must pass and check an errp, which will only be
set for a socket, and this function qemu_fopen_fd is never intended to
be used for sockets.  The file case never fails.  IMO the current code
is better.  Are you OK with keeping it?

- Steve

>> +    QIOChannel *ioc = QIO_CHANNEL(fioc);
>> +    QEMUFile *f = writable ? qemu_fopen_channel_output(ioc) :
>> +                             qemu_fopen_channel_input(ioc);
>> +    qio_channel_set_name(ioc, name);
>> +    return f;
>> +}
>> diff --git a/migration/qemu-file-channel.h b/migration/qemu-file-channel.h
>> index 0028a09..324ae2d 100644
>> --- a/migration/qemu-file-channel.h
>> +++ b/migration/qemu-file-channel.h
>> @@ -29,4 +29,10 @@
>>  
>>  QEMUFile *qemu_fopen_channel_input(QIOChannel *ioc);
>>  QEMUFile *qemu_fopen_channel_output(QIOChannel *ioc);
>> +
>> +QEMUFile *qemu_file_open(const char *path, int flags, int mode,
>> +                         const char *name, Error **errp);
>> +
>> +QEMUFile *qemu_fd_open(int fd, bool writable, const char *name);
>> +
>>  #endif
>> -- 
>> 1.8.3.1
>>
>>