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