[Qemu-devel] Re: [RFC][RESEND][PATCH v1 02/15] virtproxy: qemu-vp, standalone daemon skeleton

[Adam Litke]

On Wed, 2010-11-03 at 10:27 -0500, Michael Roth wrote:
> +/* mirror qemu I/O-related code for standalone daemon */
> +typedef struct IOHandlerRecord {
> +    int fd;
> +    IOCanReadHandler *fd_read_poll;
> +    IOHandler *fd_read;
> +    IOHandler *fd_write;
> +    int deleted;
> +    void *opaque;
> +    /* temporary data */
> +    struct pollfd *ufd;
> +    QLIST_ENTRY(IOHandlerRecord) next;
> +} IOHandlerRecord;

As you say, this is exactly the same structure as defined in vl.c.  If
you copy and paste code for a new feature you should be looking for a
way to share it instead.  Why not create a separate patch that defines
this structure in vl.h which can then be included by vl.c and virtproxy
code.

> +static QLIST_HEAD(, IOHandlerRecord) io_handlers =
> +    QLIST_HEAD_INITIALIZER(io_handlers);
> +
> +int vp_set_fd_handler2(int fd,
> +                         IOCanReadHandler *fd_read_poll,
> +                         IOHandler *fd_read,
> +                         IOHandler *fd_write,
> +                         void *opaque)
> +{
> +    IOHandlerRecord *ioh;
> +
> +    if (!fd_read && !fd_write) {
> +        QLIST_FOREACH(ioh, &io_handlers, next) {
> +            if (ioh->fd == fd) {
> +                ioh->deleted = 1;
> +                break;
> +            }
> +        }
> +    } else {
> +        QLIST_FOREACH(ioh, &io_handlers, next) {
> +            if (ioh->fd == fd)
> +                goto found;
> +        }
> +        ioh = qemu_mallocz(sizeof(IOHandlerRecord));
> +        QLIST_INSERT_HEAD(&io_handlers, ioh, next);
> +    found:
> +        ioh->fd = fd;
> +        ioh->fd_read_poll = fd_read_poll;
> +        ioh->fd_read = fd_read;
> +        ioh->fd_write = fd_write;
> +        ioh->opaque = opaque;
> +        ioh->deleted = 0;
> +    }
> +    return 0;
> +}

Copied from vl.c -- export it instead.

> +int vp_set_fd_handler(int fd,
> +                        IOHandler *fd_read,
> +                        IOHandler *fd_write,
> +                        void *opaque)
> +{
> +    return vp_set_fd_handler2(fd, NULL, fd_read, fd_write, opaque);
> +}
> +
> +int vp_send_all(int fd, const void *buf, int len1)
> +{
> +    int ret, len;
> +
> +    len = len1;
> +    while (len > 0) {
> +        ret = write(fd, buf, len);
> +        if (ret < 0) {
> +            if (errno != EINTR && errno != EAGAIN) {
> +                warn("write() failed");
> +                return -1;
> +            }
> +        } else if (ret == 0) {
> +            break;
> +        } else {
> +            buf += ret;
> +            len -= ret;
> +        }
> +    }
> +    return len1 - len;
> +}

Copied from qemu-char.c -- Share please.

> +static void main_loop_wait(int nonblocking)
> +{
> +    IOHandlerRecord *ioh;
> +    fd_set rfds, wfds, xfds;
> +    int ret, nfds;
> +    struct timeval tv;
> +    int timeout = 1000;
> +
> +    if (nonblocking) {
> +        timeout = 0;
> +    }
> +
> +    /* poll any events */
> +    nfds = -1;
> +    FD_ZERO(&rfds);
> +    FD_ZERO(&wfds);
> +    FD_ZERO(&xfds);
> +    QLIST_FOREACH(ioh, &io_handlers, next) {
> +        if (ioh->deleted)
> +            continue;
> +        if (ioh->fd_read &&
> +            (!ioh->fd_read_poll ||
> +             ioh->fd_read_poll(ioh->opaque) != 0)) {
> +            FD_SET(ioh->fd, &rfds);
> +            if (ioh->fd > nfds)
> +                nfds = ioh->fd;
> +        }
> +        if (ioh->fd_write) {
> +            FD_SET(ioh->fd, &wfds);
> +            if (ioh->fd > nfds)
> +                nfds = ioh->fd;
> +        }
> +    }
> +
> +    tv.tv_sec = timeout / 1000;
> +    tv.tv_usec = (timeout % 1000) * 1000;
> +
> +    ret = select(nfds + 1, &rfds, &wfds, &xfds, &tv);
> +
> +    if (ret > 0) {
> +        IOHandlerRecord *pioh;
> +
> +        QLIST_FOREACH_SAFE(ioh, &io_handlers, next, pioh) {
> +            if (ioh->deleted) {
> +                QLIST_REMOVE(ioh, next);
> +                qemu_free(ioh);
> +                continue;
> +            }
> +            if (ioh->fd_read && FD_ISSET(ioh->fd, &rfds)) {
> +                ioh->fd_read(ioh->opaque);
> +            }
> +            if (ioh->fd_write && FD_ISSET(ioh->fd, &wfds)) {
> +                ioh->fd_write(ioh->opaque);
> +            }
> +        }
> +    }
> +

This resembles vl.c's main_loop_wait() but I can see why you might want
your own.  There is opportunity for sharing the select logic and ioh
callbacks but I think that could be addressed later.

-- 
Thanks,
Adam