Hello Daniel, On Tue, Nov 23, 2021 at 6:56 AM Daniel P. Berrangé <berra...@redhat.com> wrote: > > On Tue, Nov 23, 2021 at 01:46:44AM -0300, Leonardo Bras Soares Passos wrote: > > Hello Daniel, > > > > On Fri, Nov 12, 2021 at 7:54 AM Daniel P. Berrangé <berra...@redhat.com> > > wrote: > > [...] > > > > @@ -561,12 +577,15 @@ static ssize_t > > > > qio_channel_socket_writev_flags(QIOChannel *ioc, > > > > retry: > > > > ret = sendmsg(sioc->fd, &msg, flags); > > > > if (ret <= 0) { > > > > - if (errno == EAGAIN) { > > > > + switch (errno) { > > > > + case EAGAIN: > > > > return QIO_CHANNEL_ERR_BLOCK; > > > > - } > > > > - if (errno == EINTR) { > > > > + case EINTR: > > > > goto retry; > > > > + case ENOBUFS: > > > > + return QIO_CHANNEL_ERR_NOBUFS; > > > > > > Why does ENOBUFS need handling separately instead of letting > > > the error_setg_errno below handle it ? > > > > > > The caller immediately invokes error_setg_errno() again, > > > just with different error message. > > > > > > No code in this series ever looks at QIO_CHANNEL_ERR_NOBUFS > > > either, so we don't even need that special error return code > > > added AFAICT ? > > > > > > > The idea was to add a custom message for ENOBUFS return when sending > > with MSG_ZEROCOPY. > > I mean, having this message is important for the user to understand > > why the migration is failing, but it would > > not make any sense to have this message while a non-zerocopy sendmsg() > > returns with ENOBUFS. > > > > ENOBUFS : The output queue for a network interface was full. This > > generally indicates that the interface has stopped sending, but may be > > caused by transient congestion. > > > > As an alternative, I could add this message inside the switch, inside > > an if (flags & MSG_ZEROCOPY) on qio_channel_socket_writev_flags() > > instead of in it's caller. > > But for me it looks bloated, I mean, dealing with an error for > > ZEROCOPY only in the general function. > > It is perfectly reasonable to check flags in this method. > > > OTOH, if you think that it's a better idea to deal with every error in > > qio_channel_socket_writev_flags() instead of in the caller, I will > > change it for v6. Please let me know. > > Yes, this method is already taking an ERror **errp parameter and > reporting a user facing error. If we need to report different > message text for ENOBUFS, it should be done in this method too. > > The reason QIO_CHANNEL_ERR_BLOCK is special is because we are > explicitly not treating it as an error scenario at all. That's > different to the ENOBUFS case. >
Ok, I will change it for v6. > > > > > > > } > > > > + > > > > error_setg_errno(errp, errno, > > > > "Unable to write to socket"); > > > > return -1; > > > > @@ -670,6 +689,127 @@ static ssize_t > > > > qio_channel_socket_writev(QIOChannel *ioc, > > > > } > > > > #endif /* WIN32 */ > > > > > > > > + > > > > +#ifdef CONFIG_LINUX > > > > + > > > > +static int qio_channel_socket_poll(QIOChannelSocket *sioc, bool > > > > zerocopy, > > > > + Error **errp) > > > > > > There's only one caller and it always passes zerocopy=true, > > > so this parmeter looks pointless. > > > > I did that for possible reuse of this function in the future: > > - As of today, this is certainly compiled out, but if at some point > > someone wants to use poll for something other > > than the reading of an zerocopy errqueue, it could be reused. > > > > But sure, if that's not desirable, I can remove the parameter (and the > > if clause for !zerocopy). > > > > > > > > > +{ > > > > + struct pollfd pfd; > > > > + int ret; > > > > + > > > > + pfd.fd = sioc->fd; > > > > + pfd.events = 0; > > > > + > > > > + retry: > > > > + ret = poll(&pfd, 1, -1); > > > > + if (ret < 0) { > > > > + switch (errno) { > > > > + case EAGAIN: > > > > + case EINTR: > > > > + goto retry; > > > > + default: > > > > + error_setg_errno(errp, errno, > > > > + "Poll error"); > > > > + return ret; > > > > > > return -1; > > > > > > > + } > > > > + } > > > > + > > > > + if (pfd.revents & (POLLHUP | POLLNVAL)) { > > > > + error_setg(errp, "Poll error: Invalid or disconnected fd"); > > > > + return -1; > > > > + } > > > > + > > > > + if (!zerocopy && (pfd.revents & POLLERR)) { > > > > + error_setg(errp, "Poll error: Errors present in errqueue"); > > > > + return -1; > > > > + } > > > > > > > + > > > > + return ret; > > > > > > return 0; > > > > In the idea of future reuse I spoke above, returning zero here would > > make this function always look like the poll timed out. Some future > > users may want to repeat the waiting if poll() timed out, or if > > (return > 0) stop polling. > > Now that I'm looking again, we should not really use poll() at all, > as GLib provides us higher level APIs. We in fact already have the > qio_channel_wait() method as a general purpose helper for waiting > for an I/O condition to occur.; > So you suggest using qio_channel_wait(sioc, G_IO_IN); instead of creating the new qio_channel_socket_poll(). Is the above correct? I mean, is it as simple as that? > > > I understand the idea of testing SO_EE_CODE_ZEROCOPY_COPIED to be able > > to tell whenever zerocopy fell back to copying for some reason, but I > > don't see how this can be helpful here. > > > > Other than that I would do rv++ instead of rv=1 here, if I want to > > keep track of how many buffers were sent with zerocopy and how many > > ended up being copied. > > Sure, we could do "ret > 0 == number of buffers that were copied" > as the API contract, rather than just treating it as a boolean. Ok, then you suggest the responsibility of checking the number of writes with SO_EE_CODE_ZEROCOPY_COPIED, comparing with the total number of writes, and deciding whether to disable or not zerocopy should be on the caller. (Disabling zerocopy on a single SO_EE_CODE_ZEROCOPY_COPIED doesn't seem a good idea also) > > > > 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 :| > Best regards, Leo