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 :|
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list