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

[Venkateswararao Jujjuri <jvrao@linux.vnet.ibm.com>]

On 05/12/2011 10:48 PM, Stefan Hajnoczi wrote:
> 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?
>
Name of the function is little weird .. it basically checking if the 
thread infrastructure
is initialized or not. One can call g_thread_init() unconditionally.. 
but this may be good
practice.

http://developer.gnome.org/glib/2.28/glib-Threads.html#g-thread-supported

We will address all your comments. Thanks for the review.

- JV

> Stefan