Re: [Qemu-devel] [PATCH] [RFC] Add glib support to main loop

[Blue Swirl]

On Wed, Jul 27, 2011 at 11:48 PM, Anthony Liguori <address@hidden> wrote:
> On 07/27/2011 03:43 PM, Blue Swirl wrote:
>>
>> On Wed, Jul 27, 2011 at 3:06 AM, Anthony Liguori<address@hidden>
>>  wrote:
>>>
>>> This allows GSources to be used to register callback events in QEMU.
>>>  This is
>>> useful as it allows us to take greater advantage of glib and also because
>>> it
>>> allows us to write code that is more easily testable outside of QEMU
>>> since we
>>> can make use of glib's main loop in unit tests.
>>>
>>> All new code should use glib's callback mechanisms for registering fd
>>> events
>>> which are very well documented at:
>>>
>>> http://developer.gnome.org/glib/stable/glib-The-Main-Event-Loop.html
>>>
>>> And:
>>>
>>> http://developer.gnome.org/gio/stable/
>>>
>>> Signed-off-by: Anthony Liguori<address@hidden>
>>> ---
>>>  vl.c |   74
>>> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>  1 files changed, 74 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/vl.c b/vl.c
>>> index 4b6688b..19774ac 100644
>>> --- a/vl.c
>>> +++ b/vl.c
>>> @@ -111,6 +111,8 @@ int main(int argc, char **argv)
>>>  #define main qemu_main
>>>  #endif /* CONFIG_COCOA */
>>>
>>> +#include<glib.h>
>>> +
>>>  #include "hw/hw.h"
>>>  #include "hw/boards.h"
>>>  #include "hw/usb.h"
>>> @@ -1309,6 +1311,75 @@ void qemu_system_vmstop_request(int reason)
>>>     qemu_notify_event();
>>>  }
>>>
>>> +static GPollFD poll_fds[1024 * 2]; /* this is probably overkill */
>>> +static int n_poll_fds;
>>> +static int max_priority;
>>> +
>>> +static void glib_select_fill(int *max_fd, fd_set *rfds, fd_set *wfds,
>>> +                             fd_set *xfds, struct timeval *tv)
>>> +{
>>> +    GMainContext *context = g_main_context_default();
>>> +    int i;
>>> +    int timeout = 0, cur_timeout;
>>> +
>>> +    g_main_context_prepare(context,&max_priority);
>>> +
>>> +    n_poll_fds = g_main_context_query(context, max_priority,&timeout,
>>> +                                      poll_fds, ARRAY_SIZE(poll_fds));
>>> +    g_assert(n_poll_fds<= ARRAY_SIZE(poll_fds));
>>
>> The use of g_assert() means that production code should define
>> G_DISABLE_ASSERT in addition to NDEBUG. Should we make both default
>> for stable versions?
>
> Oh, this is just me being lazy.  There's a way to do this without using the
> assert.  I put the assert there to make sure that I didn't leave a latent
> bug.
>
> But in general, I don't think we should ever define NDEBUG.

Why? I think it's normal practice to define NDEBUG (and
G_DISABLE_ASSERT if g_assert() is used) for production builds.

>>> +
>>> +    for (i = 0; i<  n_poll_fds; i++) {
>>> +        GPollFD *p =&poll_fds[i];
>>> +
>>> +        if ((p->events&  G_IO_IN)) {
>>> +            FD_SET(p->fd, rfds);
>>> +            *max_fd = MAX(*max_fd, p->fd);
>>> +        }
>>> +        if ((p->events&  G_IO_OUT)) {
>>> +            FD_SET(p->fd, wfds);
>>> +            *max_fd = MAX(*max_fd, p->fd);
>>> +        }
>>> +        if ((p->events&  G_IO_ERR)) {
>>> +            FD_SET(p->fd, xfds);
>>> +            *max_fd = MAX(*max_fd, p->fd);
>>> +        }
>>> +    }
>>> +
>>> +    cur_timeout = (tv->tv_sec * 1000) + ((tv->tv_usec + 500) / 1000);
>>> +    if (timeout>= 0&&  timeout<  cur_timeout) {
>>> +        tv->tv_sec = timeout / 1000;
>>> +        tv->tv_usec = (timeout % 1000) * 1000;
>>> +    }
>>> +}
>>> +
>>> +static void glib_select_poll(fd_set *rfds, fd_set *wfds, fd_set *xfds,
>>> +                             bool err)
>>> +{
>>> +    GMainContext *context = g_main_context_default();
>>> +
>>> +    if (!err) {
>>> +        int i;
>>> +
>>> +        for (i = 0; i<  n_poll_fds; i++) {
>>> +            GPollFD *p =&poll_fds[i];
>>> +
>>> +            if ((p->events&  G_IO_IN)&&  FD_ISSET(p->fd, rfds)) {
>>> +                p->revents |= G_IO_IN;
>>> +            }
>>> +            if ((p->events&  G_IO_OUT)&&  FD_ISSET(p->fd, wfds)) {
>>> +                p->revents |= G_IO_OUT;
>>> +            }
>>> +            if ((p->events&  G_IO_ERR)&&  FD_ISSET(p->fd, xfds)) {
>>> +                p->revents |= G_IO_ERR;
>>> +            }
>>> +        }
>>> +    }
>>> +
>>> +    if (g_main_context_check(context, max_priority, poll_fds,
>>> n_poll_fds)) {
>>> +        g_main_context_dispatch(context);
>>> +    }
>>> +}
>>> +
>>
>> The above functions do not seem to be referenced anywhere.
>
> They are in the code below :-)

Sorry, internal compiler error.

>>>  void main_loop_wait(int nonblocking)
>>>  {
>>>     fd_set rfds, wfds, xfds;
>>> @@ -1334,8 +1405,10 @@ void main_loop_wait(int nonblocking)
>>>     FD_ZERO(&rfds);
>>>     FD_ZERO(&wfds);
>>>     FD_ZERO(&xfds);
>>> +
>>>     qemu_iohandler_fill(&nfds,&rfds,&wfds,&xfds);
>>>     slirp_select_fill(&nfds,&rfds,&wfds,&xfds);
>>> +    glib_select_fill(&nfds,&rfds,&wfds,&xfds,&tv);
>>>
>>>     qemu_mutex_unlock_iothread();
>>>     ret = select(nfds + 1,&rfds,&wfds,&xfds,&tv);
>>> @@ -1343,6 +1416,7 @@ void main_loop_wait(int nonblocking)
>>>
>>>     qemu_iohandler_poll(&rfds,&wfds,&xfds, ret);
>>>     slirp_select_poll(&rfds,&wfds,&xfds, (ret<  0));
>>> +    glib_select_poll(&rfds,&wfds,&xfds, (ret<  0));
>>>
>>>     qemu_run_all_timers();
>
> Regards,
>
> Anthony Liguori
>
>>> --
>>> 1.7.4.1
>>>
>>>
>>>
>
>