Re: [PATCH v2 1/2] QIOChannelSocket: Reduce ifdefs to improve readability

[Leonardo Bras Soares Passos]

Hello Daniel,

On Thu, Jun 9, 2022 at 5:10 AM Daniel P. Berrangé <berrange@redhat.com> wrote:
>
> On Wed, Jun 08, 2022 at 06:04:02PM -0300, Leonardo Bras wrote:
> > During implementation of MSG_ZEROCOPY feature, a lot of #ifdefs were
> > introduced, particularly at qio_channel_socket_writev().
> >
> > Rewrite some of those changes so it's easier to read.
> >                                                                       ...
> > Signed-off-by: Leonardo Bras <leobras@redhat.com>
> > ---
> >  io/channel-socket.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/io/channel-socket.c b/io/channel-socket.c
> > index dc9c165de1..ef7c7cfbac 100644
> > --- a/io/channel-socket.c
> > +++ b/io/channel-socket.c
> > @@ -554,6 +554,7 @@ static ssize_t qio_channel_socket_writev(QIOChannel 
> > *ioc,
> >      size_t fdsize = sizeof(int) * nfds;
> >      struct cmsghdr *cmsg;
> >      int sflags = 0;
> > +    bool zero_copy_enabled = false;
> >
> >      memset(control, 0, CMSG_SPACE(sizeof(int) * SOCKET_MAX_FDS));
> >
> > @@ -581,6 +582,7 @@ static ssize_t qio_channel_socket_writev(QIOChannel 
> > *ioc,
> >  #ifdef QEMU_MSG_ZEROCOPY
> >      if (flags & QIO_CHANNEL_WRITE_FLAG_ZERO_COPY) {
> >          sflags = MSG_ZEROCOPY;
> > +        zero_copy_enabled = true;
> >      }
>
> There should be a
>
>  #else
>     error_setg(errp, "Zero copy not supported on this platform");
>     return -1;
>  #endif
>

IIUC, if done as suggested, it will break every non-zero-copy call of
qio_channel_socket_writev();

I think you are suggesting something like :

    if (flags & QIO_CHANNEL_WRITE_FLAG_ZERO_COPY) {
#ifdef QEMU_MSG_ZEROCOPY
        sflags = MSG_ZEROCOPY;
        zero_copy_enabled = true; // I know you suggested this out,
just for example purposes
#else
        error_setg(errp, "Zero copy not supported on this platform");
        return -1;
#endif
    }

Which is supposed to fail if QIO_CHANNEL_WRITE_FLAG_ZERO_COPY is specified, but
qemu does not support it at compile time.

If I get the part above correctly, it would not be necessary, as
qio_channel_socket_writev() is
called only by qio_channel_writev_full(), which tests:

    if ((flags & QIO_CHANNEL_WRITE_FLAG_ZERO_COPY) &&
        !qio_channel_has_feature(ioc, QIO_CHANNEL_FEATURE_WRITE_ZERO_COPY)) {
        error_setg_errno(errp, EINVAL,
                         "Requested Zero Copy feature is not available");
        return -1;
    }

and QIO_CHANNEL_FEATURE_WRITE_ZERO_COPY is only set in
qio_channel_socket_connect_sync(), and is conditional to QEMU_MSG_ZEROCOPY
being enabled during compile time. Meaning it's the same test as
before mentioned, but
failing earlier.

> >  #endif
> >
> > @@ -592,15 +594,13 @@ static ssize_t qio_channel_socket_writev(QIOChannel 
> > *ioc,
> >              return QIO_CHANNEL_ERR_BLOCK;
> >          case EINTR:
> >              goto retry;
> > -#ifdef QEMU_MSG_ZEROCOPY
> >          case ENOBUFS:
> > -            if (sflags & MSG_ZEROCOPY) {
> > +            if (zero_copy_enabled) {
>
> if (flags & QIO_CHANNEL_WRITE_FLAG_ZERO_COPY)
>
> avoids the #ifdef without needing to add yet another
> variable expressing what's already expressed in both
> 'flags' and 'sflags'.

Yes, it does, but at the cost of not compiling-out the zero-copy part
when it's not supported,
since the QIO_CHANNEL_WRITE_FLAG_ZERO_COPY comes as a parameter. This ends up
meaning there will be at least one extra test for every time this
function is called (the one in the next patch).

An option would be testing sflags & MSG_ZEROCOPY, which would
compile-out zero-copy code
if it's not supported, but there was a bug in some distros where
MSG_ZEROCOPY is not defined,
causing the build to fail.

I understand the idea of reusing those variables instead of creating a
new one, but this boolean
variable will most certainly be compiled-out in this function, and
will allow compiling out the
zero-copy code where it's not supported.

Best regards,
Leo


>
> >                  error_setg_errno(errp, errno,
> >                                   "Process can't lock enough memory for 
> > using MSG_ZEROCOPY");
> >                  return -1;
> >              }
> >              break;
> > -#endif
> >          }
> >
> >          error_setg_errno(errp, errno,
> > --
> > 2.36.1
> >
>
> 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 :|
>