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

[Daniel P. Berrange]

On Wed, Nov 03, 2010 at 01:57:05PM +0100, Daniel Veillard wrote:
> 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.

We explicit chose not to return errors from the lock/unlock
functions, because all but one of the errors cannot occur
in the simplified mutex API we provide. The remaining scenario
of EDEADLOCK could occur, if the default mutex impl did error
checking, but in reality the just actually deadlocks instead.
Add error checking in the mutex callers would lead to a huge
increase in code complexity for no real world benefits.

> > +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,

Yep,  saferead/write cannot be used on any FD with O_NONBLOCK
set, because they'll just spin in a 100% CPU loop whenever
EAGAIN occurs, until they get nbytes worth of data.

> [...]
> > +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 ?

Yeah, pretty trivial to add later

Daniel
-- 
|: Red Hat, Engineering, London    -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :|
|: http://autobuild.org        -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-   F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|