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

[Daniel P . Berrangé]

On Tue, May 24, 2022 at 04:14:31PM -0500, Eric Blake wrote:
> 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.

Nb, I wrote 'zero bytes' (as in "byte count equal to zero")  not
'zeroed bytes' (as in "set all bytes to the value zero").

I'll reword it to make it less confusing though.

> 
> > + */
> > +
> > +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
> 

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 :|