6:42 a.m.
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 :|