Re: [Qemu-devel] [PATCH 01/25] [virtio-9p] Add infrastructure to support glib threads and coroutines.

[Stefan Hajnoczi <stefanha@gmail.com>]

On Thu, May 12, 2011 at 9:57 PM, Venkateswararao Jujjuri (JV)
<jvrao@linux.vnet.ibm.com> wrote:
> +/* v9fs glib thread pool */
> +V9fsThPool v9fs_pool;

This should be static and an init function in this file could perform
initialization.  Right now the initialization is inlined in
virtio-9p-device.c.

> +void v9fs_qemu_submit_request(V9fsRequest *req)
> +{
> +    V9fsThPool *p = &v9fs_pool;
> +
> +    req->done = 0;
> +    p->requests = g_list_append(p->requests, req);
> +    g_thread_pool_push(v9fs_pool.pool, req, NULL);
> +}
> +
> +void v9fs_qemu_process_req_done(void *arg)
> +{
> +    struct V9fsThPool *p = &v9fs_pool;
> +    char byte;
> +    ssize_t len;
> +    GList *cur_req, *next_req;
> +
> +    do {
> +        len = read(p->rfd, &byte, sizeof(byte));
> +    } while (len == -1 && errno == EINTR);
> +
> +    for (cur_req = p->requests; cur_req != NULL; cur_req = next_req) {
> +        V9fsRequest *req = cur_req->data;
> +        next_req = g_list_next(cur_req);
> +
> +        if (!req->done) {
> +            continue;
> +        }

The requests list is only used for completion, why not enqueue only
completed requests and get rid of the done field.  Then this function
doesn't have to skip over in-flight requests.

> +        Coroutine *entry = req->coroutine;
> +        qemu_coroutine_enter(entry, NULL);
> +
> +        p->requests = g_list_delete_link(p->requests, cur_req);

Does this actually free the req?

> +static int notifier_fds[2];

Why global?

> +    if (!g_thread_supported()) {
> +        g_thread_init(NULL);
> +    }

The logic looks inverted.  But if GThread isn't supported where in big
trouble so we should probably exit?

Stefan