From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:34800) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QEjPp-0006yY-KM for qemu-devel@nongnu.org; Tue, 26 Apr 2011 10:39:02 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1QEjPo-00085n-CM for qemu-devel@nongnu.org; Tue, 26 Apr 2011 10:39:01 -0400 Received: from e7.ny.us.ibm.com ([32.97.182.137]:34308) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QEjPo-00085i-8h for qemu-devel@nongnu.org; Tue, 26 Apr 2011 10:39:00 -0400 Received: from d01relay04.pok.ibm.com (d01relay04.pok.ibm.com [9.56.227.236]) by e7.ny.us.ibm.com (8.14.4/8.13.1) with ESMTP id p3QEGYJp029533 for ; Tue, 26 Apr 2011 10:16:34 -0400 Received: from d03av02.boulder.ibm.com (d03av02.boulder.ibm.com [9.17.195.168]) by d01relay04.pok.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id p3QEcwOW061692 for ; Tue, 26 Apr 2011 10:38:58 -0400 Received: from d03av02.boulder.ibm.com (loopback [127.0.0.1]) by d03av02.boulder.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id p3QEc8Ka001664 for ; Tue, 26 Apr 2011 08:38:10 -0600 Message-ID: <4DB6D8EA.9060007@linux.vnet.ibm.com> Date: Tue, 26 Apr 2011 09:38:34 -0500 From: Michael Roth MIME-Version: 1.0 References: <1303138953-1334-1-git-send-email-mdroth@linux.vnet.ibm.com> <1303138953-1334-10-git-send-email-mdroth@linux.vnet.ibm.com> In-Reply-To: Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [RFC][PATCH v2 09/17] qmp proxy: core code for proxying qmp requests to guest List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefan Hajnoczi Cc: aliguori@linux.vnet.ibm.com, agl@linux.vnet.ibm.com, qemu-devel@nongnu.org, Jes.Sorensen@redhat.com On 04/26/2011 08:21 AM, Stefan Hajnoczi wrote: > On Mon, Apr 18, 2011 at 4:02 PM, Michael Roth wrote: >> +static int qmp_proxy_cancel_request(QmpProxy *p, QmpProxyRequest *r) >> +{ >> + if (r&& r->cb) { >> + r->cb(r->opaque, NULL, NULL); >> + } >> + >> + return 0; >> +} >> + >> +static int qmp_proxy_cancel_all(QmpProxy *p) >> +{ >> + QmpProxyRequest *r, *tmp; >> + QTAILQ_FOREACH_SAFE(r,&p->requests, entry, tmp) { >> + qmp_proxy_cancel_request(p, r); >> + QTAILQ_REMOVE(&p->requests, r, entry); >> + } >> + >> + return 0; >> +} > > qmp_proxy_cancel_all() will remove requests from the list. > qmp_proxy_cancel_request() will not remove it from the list. This > could cause confusion in the future if someone adds a call to > qmp_proxy_cancel_request() without realizing that it will not remove > the request from the list. The two function's names are similar, it > would be nice if they acted the same way. > >> +static void qmp_proxy_process_event(JSONMessageParser *parser, QList *tokens) >> +{ >> + QmpProxy *p = container_of(parser, QmpProxy, parser); >> + QmpProxyRequest *r; >> + QObject *obj; >> + QDict *qdict; >> + Error *err = NULL; >> + >> + fprintf(stderr, "qmp proxy: called\n"); >> + obj = json_parser_parse_err(tokens, NULL,&err); >> + if (!obj) { >> + fprintf(stderr, "qmp proxy: failed to parse\n"); >> + return; >> + } else { >> + fprintf(stderr, "qmp proxy: parse successful\n"); >> + qdict = qobject_to_qdict(obj); >> + } >> + >> + if (qdict_haskey(qdict, "_control_event")) { >> + /* handle transport-level control event */ >> + qmp_proxy_process_control_event(p, qdict); >> + } else if (qdict_haskey(qdict, "return")) { >> + /* handle proxied qmp command response */ >> + fprintf(stderr, "received return\n"); >> + r = QTAILQ_FIRST(&p->requests); >> + if (!r) { >> + fprintf(stderr, "received return, but no request queued\n"); > > QDECREF(qdict)? > >> + return; >> + } >> + /* XXX: can't assume type here */ >> + fprintf(stderr, "recieved response for cmd: %s\nreturn: %s\n", >> + r->name, qstring_get_str(qobject_to_json(QOBJECT(qdict)))); >> + r->cb(r->opaque, qdict_get(qdict, "return"), NULL); >> + QTAILQ_REMOVE(&p->requests, r, entry); >> + qemu_free(r); >> + fprintf(stderr, "done handling response\n"); >> + } else { >> + fprintf(stderr, "received invalid payload format\n"); >> + } >> + >> + QDECREF(qdict); >> +} >> +void qmp_proxy_send_request(QmpProxy *p, const char *name, >> + const QDict *args, Error **errp, >> + QmpGuestCompletionFunc *cb, void *opaque) >> +{ >> + QmpProxyRequest *r = qemu_mallocz(sizeof(QmpProxyRequest)); >> + QDict *payload = qdict_new(); >> + QString *json; >> + >> + /* TODO: don't really need to hold on to name/args after encoding */ >> + r->name = name; >> + r->args = args; >> + r->cb = cb; >> + r->opaque = opaque; >> + >> + qdict_put_obj(payload, "execute", QOBJECT(qstring_from_str(r->name))); >> + /* TODO: casting a const so we can add it to our dictionary. bad. */ >> + qdict_put_obj(payload, "arguments", QOBJECT((QDict *)args)); >> + >> + json = qobject_to_json(QOBJECT((QDict *)payload)); >> + if (!json) { >> + goto out_bad; >> + } >> + >> + QTAILQ_INSERT_TAIL(&p->requests, r, entry); >> + g_string_append(p->tx, qstring_get_str(json)); >> + QDECREF(json); >> + qmp_proxy_write(p); >> + return; >> + >> +out_bad: >> + cb(opaque, NULL, NULL); >> + qemu_free(r); > > Need to free payload? > >> +} >> + >> +QmpProxy *qmp_proxy_new(CharDriverState *chr) >> +{ >> + QmpProxy *p = qemu_mallocz(sizeof(QmpProxy)); >> + >> + signal_init(&guest_agent_up_event); >> + signal_init(&guest_agent_reset_event); >> + >> + /* there's a reason for this madness */ > > Helpful comment :) > >> + p->tx_timer = qemu_new_timer(rt_clock, qmp_proxy_write_handler, p); >> + p->tx_timer_interval = 10; >> + p->tx = g_string_new(""); >> + p->chr = chr; >> + json_message_parser_init(&p->parser, qmp_proxy_process_event); >> + QTAILQ_INIT(&p->requests); >> + >> + return p; >> +} >> + >> +void qmp_proxy_close(QmpProxy *p) >> +{ >> + qmp_proxy_cancel_all(p); >> + g_string_free(p->tx, TRUE); > > Free tx_timer? > >> + qemu_free(p); >> +} All good catches/suggestions, thanks.