Re: [Qemu-devel] [RFC][PATCH v2 09/17] qmp proxy: core code for proxying qmp requests to guest

[Michael Roth <mdroth@linux.vnet.ibm.com>]

On 04/26/2011 08:21 AM, Stefan Hajnoczi wrote:
> On Mon, Apr 18, 2011 at 4:02 PM, Michael Roth<mdroth@linux.vnet.ibm.com>  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.