Re: [PATCH 01/20] io: add a QIOChannelNull equivalent to /dev/null

[Eric Blake]

On Tue, May 24, 2022 at 12:02:16PM +0100, Daniel P. Berrangé wrote:
> This is for code which needs a portable equivalent to a QIOChannelFile
> connected to /dev/null.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  include/io/channel-null.h         |  55 +++++++
>  io/channel-null.c                 | 237 ++++++++++++++++++++++++++++++
>  io/meson.build                    |   1 +
>  io/trace-events                   |   3 +
>  tests/unit/meson.build            |   1 +
>  tests/unit/test-io-channel-null.c |  95 ++++++++++++
>  6 files changed, 392 insertions(+)
>  create mode 100644 include/io/channel-null.h
>  create mode 100644 io/channel-null.c
>  create mode 100644 tests/unit/test-io-channel-null.c

> +/**
> + * QIOChannelNull:
> + *
> + * The QIOChannelNull object provides a channel implementation
> + * that discards all writes and returns zero bytes for all reads.

That describes the behavior of /dev/zero, not /dev/null, where reads
always fail with EOF.

> + */
> +
> +struct QIOChannelNull {
> +    QIOChannel parent;
> +    bool closed;
> +};
> +

> diff --git a/io/channel-null.c b/io/channel-null.c

> +
> +static ssize_t
> +qio_channel_null_readv(QIOChannel *ioc,
> +                       const struct iovec *iov,
> +                       size_t niov,
> +                       int **fds G_GNUC_UNUSED,
> +                       size_t *nfds G_GNUC_UNUSED,
> +                       Error **errp)
> +{
> +    QIOChannelNull *nioc = QIO_CHANNEL_NULL(ioc);
> +
> +    if (nioc->closed) {
> +        error_setg_errno(errp, EINVAL,
> +                         "Channel is closed");
> +        return -1;
> +    }
> +
> +    return 0;
> +}

But this behavior is returning early EOF instead of using iov_memset()
to read all zeroes the way /dev/zero would.

> +++ b/tests/unit/test-io-channel-null.c

> +static void test_io_channel_null_io(void)
> +{
> +    g_autoptr(QIOChannelNull) null = qio_channel_null_new();
> +    char buf[1024];
> +    GIOCondition gotcond = 0;
> +    Error *local_err = NULL;
> +
> +    g_assert(qio_channel_write(QIO_CHANNEL(null),
> +                               "Hello World", 11,
> +                               &error_abort) == 11);

I still cringe seeing tests inside g_assert(), but this is not the
first instance of it.

> +
> +    g_assert(qio_channel_read(QIO_CHANNEL(null),
> +                              buf, sizeof(buf),
> +                              &error_abort) == 0);

Okay, you're testing for /dev/null behavior of early EOF.

Other than misleading comments, this looks reasonable.  But those
comments are core enough as to what this channel does that I don't
feel comfortable giving R-b yet.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org