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

[Michael Roth <mdroth@linux.vnet.ibm.com>]

On 11/03/2010 05:47 PM, Adam Litke wrote:
> 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.
>

Yup these are all basically copy/pastes from vl.c. It feels a bit dirty, 
but I modeled things after the other qemu tools like qemu-nbd/qemu-io, 
which don't link against vl.c (and adding a target for tools to do so 
looks like it'd be a bit hairy since vl.c touches basically everything).

It might still make sense to share things like structs...but the ones 
I'm stealing here are specific to reproducing the main_loop_wait logic. 
So I guess the real question is whether main_loop_wait and friends make 
sense to expose as a utility function of some sort, and virtproxy seems 
to be the only use case so far.