Re: [Qemu-devel] [PATCH v3 05/10] Introduce QemuEvent abstraction

[Jan Kiszka]

On 2012-04-05 13:23, Paolo Bonzini wrote:
> Il 05/04/2012 12:59, Jan Kiszka ha scritto:
>> Provide generic services for binary events. Blocking wait would be
>> feasible but is not included yet as there are no users.
>>
>> diff --git a/qemu-event-posix.c b/qemu-event-posix.c
>> new file mode 100644
>> index 0000000..6138168
>> --- /dev/null
>> +++ b/qemu-event-posix.c
>> @@ -0,0 +1,110 @@
>> +/*
>> + * Posix implementations of event signaling service
>> + *
>> + * Copyright Red Hat, Inc. 2012
>> + * Copyright Siemens AG 2012
>> + *
>> + * Author:
>> + *  Paolo Bonzini <address@hidden>
>> + *  Jan Kiszka <address@hidden>
>> + *
>> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
>> + * See the COPYING file in the top-level directory.
>> + *
>> + */
>> +#include "qemu-thread.h"
>> +#include "qemu-common.h"
>> +#include "main-loop.h"
>> +
>> +#ifdef CONFIG_EVENTFD
>> +#include <sys/eventfd.h>
>> +#endif
>> +
>> +void qemu_event_init(QemuEvent *event, bool signaled)
>> +{
>> +    int fds[2];
>> +    int ret;
>> +
>> +#ifdef CONFIG_EVENTFD
>> +    ret = eventfd(signaled, EFD_NONBLOCK | EFD_CLOEXEC);
>> +    if (ret >= 0) {
>> +        event->rfd = ret;
>> +        event->wfd = dup(ret);
>> +        if (event->wfd < 0) {
>> +            qemu_error_exit(errno, __func__);
>> +        }
>> +        qemu_set_cloexec(event->wfd);
>> +        return;
>> +    }
>> +    if (errno != ENOSYS) {
>> +        qemu_error_exit(errno, __func__);
>> +    }
>> +    /* fall back to pipe-based support */
>> +#endif
>> +
>> +    ret = qemu_pipe(fds);
>> +    if (ret < 0) {
>> +        qemu_error_exit(errno, __func__);
>> +    }
>> +    event->rfd = fds[0];
>> +    event->wfd = fds[1];
>> +    if (signaled) {
>> +        qemu_event_signal(event);
>> +    }
>> +}
>> +
>> +void qemu_event_destroy(QemuEvent *event)
>> +{
>> +    close(event->rfd);
>> +    close(event->wfd);
>> +}
>> +
>> +int qemu_event_get_signal_fd(QemuEvent *event)
>> +{
>> +    return event->wfd;
>> +}
> 
> How would you use it?  Since qemu_event_signal ignores EAGAIN, polling
> for writeability of the fd is useless.

vhost, see patch 9.

> 
> This is really little more than search-and-replace from what gets out of
> event-notifier.c after my patches, and the commit message does not
> explain the differences in the two APIs.  Having separate commits as I
> had would make the steps obvious.  Is it really worthwhile to do this
> and introduce the need for patches 9+10 (plus IIRC conflicts in qemu-kvm)?

Will reorganize the patches to morph & split event-notifier.c. The
conflict with qemu-kvm is minimal, though.

> 
> (In fact, qemu_event_get_poll_fd is a layering violation too.  In a
> perfect world, aio.c would simply get a list of EventNotifiers and no
> other types of fd).

The actual problem is that we do not have an abstraction of a handle
that can be registered with a generic polling service. So we have
qemu_aio_set_fd_handler, qemu_set_fd_handler2, qemu_add_wait_object etc.
That requires the introduction of qemu_event_set_handler - and makes the
service dependent on the main loop. I'm not happy about this either,
maybe it's worth rethinking this.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux