[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



On Mon, Nov 01, 2010 at 02:38:14PM -0600, Eric Blake wrote:
> On 11/01/2010 10:12 AM, 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
> > ---
> >  include/libvirt/virterror.h |    3 +-
> >  src/Makefile.am             |    1 +
> >  src/fdstream.c              |  472 +++++++++++++++++++++++++++++++++++++++++++
> >  src/fdstream.h              |   44 ++++
> >  src/libvirt_private.syms    |    7 +
> >  src/qemu/qemu_driver.c      |  284 +-------------------------
> >  src/util/virterror.c        |    3 +
> >  7 files changed, 534 insertions(+), 280 deletions(-)
> >  create mode 100644 src/fdstream.c
> >  create mode 100644 src/fdstream.h
> > 
> > diff --git a/include/libvirt/virterror.h b/include/libvirt/virterror.h
> > index 94d686c..abf6945 100644
> > --- a/include/libvirt/virterror.h
> > +++ b/include/libvirt/virterror.h
> > @@ -73,7 +73,8 @@ typedef enum {
> >      VIR_FROM_NWFILTER,  /* Error from network filter driver */
> >      VIR_FROM_HOOK,      /* Error from Synchronous hooks */
> >      VIR_FROM_DOMAIN_SNAPSHOT, /* Error from domain snapshot */
> > -    VIR_FROM_AUDIT      /* Error from auditing subsystem */
> > +    VIR_FROM_AUDIT,     /* Error from auditing subsystem */
> > +    VIR_FROM_STREAMS,   /* Error from I/O streams */
> >  } virErrorDomain;
> 
> Is the switch from C89 style (no trailing comma) to the C99 style
> (optional trailing comma permitted) intentional?  Personally, I like it,
> since adding a new value at the end no longer requires a random-looking
> diff of the previous entry just to add a comma.

IMHO, leaving off the comma needlessly enlarges future patches 
because 2 lines have to be changed to add an entry instead 
of just one.

> > +++ b/src/fdstream.c
> > +#include <config.h>
> > +
> > +#include <sys/types.h>
> > +#include <sys/stat.h>
> > +#include <fcntl.h>
> > +#include <unistd.h>
> > +#include <sys/socket.h>
> 
> These are okay,
> 
> > +#include <sys/un.h>
> 
> but this is missing on Mingw, with no gnulib replacement as of yet.  Do
> we need to add some HAVE_SYS_UN_H checks?

Yes, i guess so.

> 
> > +#include <netinet/in.h>
> > +#include <netinet/tcp.h>
> 
> Likewise for HAVE_NETINET_TCP_H.

Doesn't gnulib take care of TCP socket portability for us ?


> > +static int
> > +virFDStreamClose(virStreamPtr st)
> > +{
> > +    struct virFDStreamData *fdst = st->privateData;
> > +
> > +    if (!fdst)
> > +        return 0;
> > +
> > +    virMutexLock(&fdst->lock);
> > +
> > +    virFDStreamFree(fdst);
> 
> Before freeing the stream, should this use VIR_CLOSE(fdst->fd) and
> return any close failures to the caller?

virFDStreamFree is what actually closes the FD in this case,
and could return an error status to the caller.

> 
> > +static int virFDStreamWrite(virStreamPtr st, const char *bytes, size_t nbytes)
> 
> Should this return ssize_t...

No, this has to return an int, to match the public API calling
convention.

> 
> > +{
> > +    struct virFDStreamData *fdst = st->privateData;
> > +    int ret;
> 
> and s/int/ssize_t/...
> 
> > +
> > +    if (!fdst) {
> > +        streamsReportError(VIR_ERR_INTERNAL_ERROR,
> > +                           "%s", _("stream is not open"));
> > +        return -1;
> > +    }
> > +
> > +    virMutexLock(&fdst->lock);
> > +
> > +retry:
> > +    ret = write(fdst->fd, bytes, nbytes);
> 
> ...to avoid (theoretical) truncation from ssize_t to int?

It doesn't help, because the API has to return 'int' regardless.

> > +
> > +
> > +int virFDStreamConnectUNIX(virStreamPtr st,
> > +                           const char *path,
> > +                           bool abstract)
> 
> Should this code be conditionally compiled for Linux, and omitted on mingw?

Yep, probably should.

> > +int virFDStreamOpenFile(virStreamPtr st,
> > +                        const char *path,
> > +                        int flags)
> > +{
> > +    int fd = open(path, flags);
> 
> This should check that (flags & O_CREAT) == 0, so as to avoid the kernel
> interpreting a third argument of garbage mode flags if a broken caller
> passes O_CREAT.
> 
> > +    /* Thanks to the POSIX i/o model, we can't reliably get
> > +     * non-blocking I/O on block devs/regular files. To
> > +     * support those we need to fork a helper process todo
> > +     * the I/O so we just have a fifo. Or use AIO :-(
> > +     */
> > +    if ((st->flags & VIR_STREAM_NONBLOCK) &&
> > +        (!S_ISCHR(sb.st_mode) &&
> > +         !S_ISFIFO(sb.st_mode))) {
> 
> Should we also permit S_ISSOCK as an fd that supports reliable
> non-blocking behavior?

AFAIK, there's no way to open a socket as a path on the
filesystem is there ?  So there'd be no way that open(path)
could return an FD for which S_ISSOCK() is true.

> > +int virFDStreamCreateFile(virStreamPtr st,
> > +                          const char *path,
> > +                          int flags,
> > +                          mode_t mode)
> > +{
> > +    int fd = open(path, flags, mode);
> 
> Except for the difference in open() calls, this looks identical to
> virFDStreamOpenFile; can they share implementations?

When I fix the code to deal with non-blocking I/O on regular
files, they will probably need some different handling in
each case.

> > +++ b/src/util/virterror.c
> > @@ -190,6 +190,9 @@ static const char *virErrorDomainName(virErrorDomain domain) {
> >          case VIR_FROM_AUDIT:
> >              dom = "Audit";
> >              break;
> > +        case VIR_FROM_STREAMS:
> > +            dom = "Streams ";
> 
> In just this context, I wondered why the trailing space?  Then looking
> at the entire file, I instead wonder: why are VIR_FROM_NWFILTER and
> VIR_FROM_AUDIT the only ones that lack trailing space?

Looks like a bug to me.


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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]