3:38 p.m.
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.
A rough heuristic analysis says that we have:
$ git grep -l enum -- '*.h' '*.h.in' | xargs sed -n '/enum/,/}/p'
\
| grep -B1 } |grep -v -- -- | grep -v } | wc -l
535
535 enum declarations, of which:
$ git grep -l enum -- '*.h' '*.h.in' | xargs sed -n '/enum/,/}/p'
\
| grep -B1 } |grep -v -- -- | grep -v } | grep , | wc -l
281
at least 281 of them end with a trailing comma (more than half, but not
by much). I'm not quite sure how to write a syntax-checker to enforce
it, though.
At any rate, that's just style (we already require C99 for other
reasons, so it doesn't affect the validity of your patch).
+++ 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?
+#include <netinet/in.h>
+#include <netinet/tcp.h>
Likewise for HAVE_NETINET_TCP_H.
+
+static void virFDStreamEvent(int watch ATTRIBUTE_UNUSED,
+ int fd ATTRIBUTE_UNUSED,
+ int events,
+ void *opaque)
+{
+ cb(stream, events, cbopaque);
+
+ virMutexLock(&fdst->lock);
+ fdst->dispatching = 0;
+ if (fdst->cbRemoved && ff)
+ (ff)(cbopaque);
Two different function pointer call styles here.
+ virMutexUnlock(&fdst->lock);
+}
+
+static int
+virFDStreamAddCallback(virStreamPtr st,
+ int events,
+ virStreamEventCallback cb,
+ void *opaque,
+ virFreeCallback ff)
Spacing is off due to the rename from qemu to vir.
+
+ if ((fdst->watch = virEventAddHandle(fdst->fd,
+ events,
+ virFDStreamEvent,
+ st,
+ NULL)) < 0) {
Likewise.
+
+static void virFDStreamFree(struct virFDStreamData *fdst)
+{
+ if (fdst->fd != -1)
+ close(fdst->fd);
This should use VIR_FORCE_CLOSE(fdst->fd).
+ VIR_FREE(fdst);
+}
+
+
+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?
+static int virFDStreamWrite(virStreamPtr st, const char *bytes,
size_t nbytes)
Should this return ssize_t...
+{
+ 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?
> +
> +static int virFDStreamRead(virStreamPtr st, char *bytes, size_t nbytes)
+{
+ struct virFDStreamData *fdst = st->privateData;
+ int ret;
Likewise.
+
+
+int virFDStreamConnectUNIX(virStreamPtr st,
+ const char *path,
+ bool abstract)
Should this code be conditionally compiled for Linux, and omitted on mingw?
+
+error:
+ close(fd);
VIR_FORCE_CLOSE(fd);
+ return -1;
+}
+
+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?
+
+error:
+ close(fd);
VIR_FORCE_CLOSE(fd);
+ return -1;
+}
+
+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?
+++ b/src/fdstream.h
+# include "internal.h"
+# include <stdbool.h>
...
+int virFDStreamCreateFile(virStreamPtr st,
+ const char *path,
+ int flags,
+ mode_t mode);
Should you also include <sys/types.h> explicitly in this file for
mode_t, since I don't see it directly listed in internal.h?
+
+#endif /* __VIR_FDSTREAM_H_ */
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index cf64bd3..fc9021a 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -335,6 +335,13 @@ virEventUpdateTimeout;
virClose;
+# fdstream.h
+virFDStreamOpen;
+virFDStreamConnectUNIX;
+virFDStreamOpenFile;
+virFDStreamCreateFile;
Alphabetically, this should be listed between event.h and files.h, and
not files.h and hash.h.
+++ 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?
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org