Re: [libvirt] [PATCH 05/11] Add a generic internal API for handling any FD based stream

On Tue, Nov 02, 2010 at 05:49:09PM +0000, Daniel P. Berrange wrote: > To avoid the need for duplicating implementations of virStream > drivers, provide a generic implementation that can handle any > FD based stream. This code is copied from the existing impl > in the QEMU driver, with the locking moved into the stream > impl, and addition of a read callback > > The FD stream code will refuse to operate on regular files or > block devices, since those can't report EAGAIN properly when > they would block on I/O > > * include/libvirt/virterror.h, include/libvirt/virterror.h: Add > VIR_FROM_STREAM error domain > * src/qemu/qemu_driver.c: Remove code obsoleted by the new > generic streams driver. > * src/fdstream.h, src/fdstream.c, src/fdstream.c, > src/libvirt_private.syms: Generic reusable FD based streams [...] > + > + virMutexLock(&fdst->lock); Somehow unrelated, but as I was going through the patch to check mostly on error handling it appeard to me we kept all the virMutex functions void, and in the pthread implementation we discard any return value to pthread_mutex_lock/unlock ... I wonder if errors should not be caught in those sub functions, and the erro stored in the virMutex structure itself.

> +retry: > + ret = read(fdst->fd, bytes, nbytes); > + if (ret < 0) { > + if (errno == EAGAIN || errno == EWOULDBLOCK) { > + ret = -2; > + } else if (errno == EINTR) { > + goto retry; > + } else { > + ret = -1; > + virReportSystemError(errno, "%s", > + _("cannot read from stream")); > + } > + } I was wondering about reusing saferead/write but they have a different semantic on blocking,

[...] > +int virFDStreamOpen(virStreamPtr st, > + int fd) [...] > +#if HAVE_SYS_UN_H > +int virFDStreamConnectUNIX(virStreamPtr st, > + const char *path, > + bool abstract) [...] > +int virFDStreamOpenFile(virStreamPtr st, > + const char *path, > + int flags) I was wondering about remote socket, not needed right now but should be doable too, right ?