Re: [PATCH V8 02/39] migration: qemu file wrappers

[Steven Sistare]

On 6/16/2022 10:55 AM, Marc-André Lureau wrote:
> Hi
> 
> On Wed, Jun 15, 2022 at 6:54 PM Steve Sistare <steven.sistare@oracle.com 
> <mailto:steven.sistare@oracle.com>> wrote:
> 
>     Add qemu_file_open and qemu_fd_open to create QEMUFile objects for unix
>     files and file descriptors.
> 
> File descriptors are not really unix specific, but that's a detail.

OK, I will change the description.

> The names of the functions in the summary do not match the code, also details 
> :)

Yup, will fix.

> Eventually, I would suggest to follow the libc fopen/fdopen naming, if that 
> makes sense. (or the QIOChannel naming)

OK. I'll use the names that Daniel suggests.

>     Signed-off-by: Steve Sistare <steven.sistare@oracle.com 
> <mailto: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..cc5aebc 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_fopen_file(const char *path, int flags, int mode,
>     +                          const char *name, Error **errp)
>     +{
> 
> 
> I would add ERR_GUARD();

error.h advises us not to clutter the code with ERRP_GUARD when it is not 
needed.

>     +    g_autoptr(QIOChannelFile) fioc = NULL;
>     +    QIOChannel *ioc;
>     +    QEMUFile *f;
>     +
>     +    if (flags & O_RDWR) {
>     +        error_setg(errp, "qemu_fopen_file %s: O_RDWR not supported", 
> path);
>     +        return NULL;
>     +    }
> 
> 
> Why not take a "bool writable" instead, like the fdopen below?

I will ditch the bools and expand the function names as Daniel suggests.

>     +
>     +    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;
> 
> 
> "f" and parentheses are kinda superfluous

OK, will fix.

>     +}
>     +
>     +QEMUFile *qemu_fopen_fd(int fd, bool writable, const char *name)
>     +{
>     +    g_autoptr(QIOChannelFile) fioc = qio_channel_file_new_fd(fd);
>     +    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;
> 
> 
> or:
> 
> g_autoptr(QIOChannelFile) fioc = qio_channel_file_new_fd(fd);
> qio_channel_set_name(QIO_CHANNEL(fioc), name);
> return writable ? qemu_fopen_channel_output(ioc) : 
> qemu_fopen_channel_input(ioc);

OK:
    g_autoptr(QIOChannelFile) fioc = qio_channel_file_new_fd(fd);
    qio_channel_set_name(QIO_CHANNEL(fioc), name);
    return writable ? qemu_fopen_channel_output(QIO_CHANNEL(fioc)) :
                      qemu_fopen_channel_input(QIO_CHANNEL(fioc));

- Steve

>     +}
>     diff --git a/migration/qemu-file-channel.h b/migration/qemu-file-channel.h
>     index 0028a09..75fd0ad 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_fopen_file(const char *path, int flags, int mode,
>     +                         const char *name, Error **errp);
>     +
>     +QEMUFile *qemu_fopen_fd(int fd, bool writable, const char *name);
>     +
>      #endif
>     -- 
>     1.8.3.1
> 
> 
> 
> 
> -- 
> Marc-André Lureau