[Qemu-devel] Re: [RFC][PATCH v5 03/21] virtagent: common code for managing client/server rpc jobs

[Adam Litke]

On Fri, 2010-12-03 at 12:03 -0600, Michael Roth wrote: 
> +/* create new client job and then put it on the queue. this can be
> + * called externally from virtagent. Since there can only be one virtagent
> + * instance we access state via an object-scoped global rather than pass
> + * it around.
> + *
> + * if this is successful virtagent will handle cleanup of req_xml after
> + * making the appropriate callbacks, otherwise callee should handle it
> + */

Explain please. Do you mean caller should handle it? Are you trying to
say that this function, when successful, "steals" the reference to
req_xml?

> +int va_client_job_add(xmlrpc_mem_block *req_xml, VAClientCallback *cb,
> +                      MonitorCompletion *mon_cb, void *mon_data)
> +{
> +    int ret;
> +    VAClientJob *client_job;
> +    TRACE("called");
> +
> +    client_job = va_client_job_new(req_xml, cb, mon_cb, mon_data);
> +    if (client_job == NULL) {
> +        return -EINVAL;
> +    }
> +
> +    ret = va_push_client_job(client_job);
> +    if (ret != 0) {
> +        LOG("error adding client to queue: %s", strerror(ret));
> +        qemu_free(client_job);
> +        return ret;
> +    }
> +
> +    return 0;
> +}
> +
> +/* create new server job and then put it on the queue in wait state
> + * this should only be called from within our read handler callback
> + */

Since this function is only 4 lines and has only one valid call site.
perhaps its better to fold it directly into the read handler callback.

> +static int va_server_job_add(xmlrpc_mem_block *resp_xml)
> +{
> +    VAServerJob *server_job;
> +    TRACE("called");
> +
> +    server_job = va_server_job_new(resp_xml);
> +    assert(server_job != NULL);
> +    va_push_server_job(server_job);
> +    return 0;
> +}

-- 
Thanks,
Adam