Re: [Qemu-devel] [PATCH v3 8/9] qga: process_event() simplification

[Marc-André Lureau]

Hi
On Tue, Aug 28, 2018 at 2:05 AM Michael Roth <address@hidden> wrote:
>
> Quoting Marc-André Lureau (2018-08-25 08:57:23)
> > Simplify the code around qmp_dispatch():
> > - rely on qmp_dispatch/check_obj() for message checking
> > - have a single send_response() point
> > - constify send_response() argument
> >
> > It changes a couple of error messages:
> >
> > * When @req isn't a dictionary, from
> >     Invalid JSON syntax
> >   to
> >     QMP input must be a JSON object
> >
> > * When @req lacks member "execute", from
> >     this feature or command is not currently supported
> >   to
> >     QMP input lacks member 'execute'
> >
> > CC: Michael Roth <address@hidden>
> > Signed-off-by: Marc-André Lureau <address@hidden>
> > ---
> >  qga/main.c | 47 +++++++++--------------------------------------
> >  1 file changed, 9 insertions(+), 38 deletions(-)
> >
> > diff --git a/qga/main.c b/qga/main.c
> > index 6d70242d05..f0ec035996 100644
> > --- a/qga/main.c
> > +++ b/qga/main.c
> > @@ -544,15 +544,15 @@ fail:
> >  #endif
> >  }
> >
> > -static int send_response(GAState *s, QDict *payload)
> > +static int send_response(GAState *s, const QDict *rsp)
> >  {
> >      const char *buf;
> >      QString *payload_qstr, *response_qstr;
> >      GIOStatus status;
> >
> > -    g_assert(payload && s->channel);
> > +    g_assert(rsp && s->channel);
> >
> > -    payload_qstr = qobject_to_json(QOBJECT(payload));
> > +    payload_qstr = qobject_to_json(QOBJECT(rsp));
> >      if (!payload_qstr) {
> >          return -EINVAL;
> >      }
> > @@ -578,53 +578,24 @@ static int send_response(GAState *s, QDict *payload)
> >      return 0;
> >  }
> >
> > -static void process_command(GAState *s, QDict *req)
> > -{
> > -    QDict *rsp;
> > -    int ret;
> > -
> > -    g_assert(req);
> > -    g_debug("processing command");
> > -    rsp = qmp_dispatch(&ga_commands, QOBJECT(req), false);
> > -    if (rsp) {
> > -        ret = send_response(s, rsp);
> > -        if (ret < 0) {
> > -            g_warning("error sending response: %s", strerror(-ret));
> > -        }
> > -        qobject_unref(rsp);
> > -    }
>
> We used to check here for the success-response=false scenario (e.g.
> guest-shutdown qga command). Now we pass the result of qmp_dispatch()
> directly to send_response(), which looks like that might cause an
> assertion now. Do we need to add handling for this?

Good catch! fixing it:

end:
    if (rsp) {
        ret = send_response(s, rsp);
        if (ret < 0) {
            g_warning("error sending error response: %s", strerror(-ret));
        }
        qobject_unref(rsp);
    }

ack, with that?

> > -}
> > -
> >  /* handle requests/control events coming in over the channel */
> >  static void process_event(void *opaque, QObject *obj, Error *err)
> >  {
> >      GAState *s = opaque;
> > -    QDict *req, *rsp;
> > +    QDict *rsp;
> >      int ret;
> >
> >      g_debug("process_event: called");
> >      assert(!obj != !err);
> >      if (err) {
> > -        goto err;
> > -    }
> > -    req = qobject_to(QDict, obj);
> > -    if (!req) {
> > -        error_setg(&err, "Input must be a JSON object");
> > -        goto err;
> > -    }
> > -    if (!qdict_haskey(req, "execute")) {
> > -        g_warning("unrecognized payload format");
> > -        error_setg(&err, QERR_UNSUPPORTED);
> > -        goto err;
> > +        rsp = qmp_error_response(err);
> > +        goto end;
> >      }
> >
> > -    process_command(s, req);
> > -    qobject_unref(obj);
> > -    return;
> > +    g_debug("processing command");
> > +    rsp = qmp_dispatch(&ga_commands, obj, false);
> >
> > -err:
> > -    g_warning("failed to parse event: %s", error_get_pretty(err));
> > -    rsp = qmp_error_response(err);
> > +end:
> >      ret = send_response(s, rsp);
> >      if (ret < 0) {
> >          g_warning("error sending error response: %s", strerror(-ret));
> > --
> > 2.18.0.547.g1d89318c48
> >
>


-- 
Marc-André Lureau