Re: [PATCH v4 2/3] QIOChannelSocket: Implement io_writev_zerocopy & io_flush_zerocopy for CONFIG_LINUX

[Peter Xu]

On Sat, Oct 09, 2021 at 04:56:12AM -0300, Leonardo Bras wrote:
> @@ -154,6 +161,17 @@ int qio_channel_socket_connect_sync(QIOChannelSocket 
> *ioc,
>          return -1;
>      }
>  
> +#ifdef CONFIG_LINUX
> +    ret = qemu_setsockopt(fd, SOL_SOCKET, SO_ZEROCOPY, &v, sizeof(v));
> +    if (ret < 0) {
> +        /* Zerocopy not available on host */
> +        return 0;
> +    }
> +
> +    qio_channel_set_feature(QIO_CHANNEL(ioc),
> +                            QIO_CHANNEL_FEATURE_WRITE_ZEROCOPY);

This is okay I think, but looks a bit weird.  Maybe nicer to be written as:

#if LINUX
      ret = setsockopt();
      if (ret == 0) {
          qio_channel_set_feature(...);
      }
#endif
      return 0;

?

> +#endif
> +
>      return 0;
>  }

[...]

> +static ssize_t qio_channel_socket_writev_zerocopy(QIOChannel *ioc,
> +                                                  const struct iovec *iov,
> +                                                  size_t niov,
> +                                                  Error **errp)
> +{
> +    QIOChannelSocket *sioc = QIO_CHANNEL_SOCKET(ioc);
> +    ssize_t ret;
> +
> +    ret = qio_channel_socket_writev_flags(ioc, iov, niov, NULL, 0,
> +                                          MSG_ZEROCOPY, errp);
> +    if (ret == QIO_CHANNEL_ERR_NOBUFS) {
> +        if (errp && *errp) {

Hmm this seems wrong, *errp should be NULL in most cases, meanwhile I think
error_setg*() takes care of errp==NULL too, so maybe we can drop this?

> +            error_setg_errno(errp, errno,
> +                             "Process can't lock enough memory for using 
> MSG_ZEROCOPY");
> +        }
> +        return -1;
> +    }
> +
> +    sioc->zerocopy_queued++;
> +    return ret;
> +}

-- 
Peter Xu