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

[Leonardo Bras Soares Passos <leobras@redhat.com>]

On Tue, Jun 14, 2022 at 5:36 AM Daniel P. Berrangé <berrange@redhat.com> wrote:
>
> On Mon, Jun 13, 2022 at 06:21:18PM -0300, Leonardo Bras Soares Passos wrote:
> > On Fri, Jun 10, 2022 at 5:25 AM Daniel P. Berrangé <berrange@redhat.com> wrote:
> > >
> >
> > [...]
> >
> > > Ok, so if it is checked earlier then we merely need an assert.
> > >
> > >      if (flags & QIO_CHANNEL_WRITE_FLAG_ZERO_COPY) {
> > >  #ifdef QEMU_MSG_ZEROCOPY
> > >          sflags = MSG_ZEROCOPY;
> > >          zero_copy_enabled = true;
> > >  #else
> > >          g_assert_unreachable();
> > >  #endif
> > > >     }
> >
> > Ok, I will add that in the next version.
> >
> > >
> > >
> > >
> > > > > > @@ -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).
> > >
> > > The cost of a simple bit test is between negligible-and-non-existant
> > > with branch prediction. I doubt it would be possible to even measure
> > > it.
> >
> > Yeah, you are probably right on that.
> > So the main learning point here is that it's not worth creating a new
> > boolean for compiling-out
> > code that should not impact performance ?
>
> As ever "it depends" so there's no hard rule, and sometimes it can
> verge on bikeshed colouring :-)
>
> I didn't like the variable in this case, because it introduces a 3rd
> variable to the method for representing whether zero copy is need,
> which is excessive. I'm not a fan of redundancy as it can often then
> lead to inconsistency. So it would need a compelling reason why it is
> better, which is difficult for such a simple method. If the code was
> more complex, a variable might have benefit of clarity, but in this
> case IMHO it was just overkill.

I see. Thanks for the clarification!

Best regards,
Leo

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