[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH 06/10] Add a generic internal API for handling any FD based stream



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 redhat com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org

Attachment: signature.asc
Description: OpenPGP digital signature


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]