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@xxxxxxxxxx +1-801-349-2682
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list