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 <mdroth@linux.vnet.ibm.com>
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
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);
- }
-}
-
/* 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
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 <mdroth@linux.vnet.ibm.com>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
> 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?
> -}
> -
> /* 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
>
Hi
On Tue, Aug 28, 2018 at 2:05 AM Michael Roth <mdroth@linux.vnet.ibm.com> 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 <mdroth@linux.vnet.ibm.com>
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > ---
> > 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
Quoting Marc-André Lureau (2018-08-28 06:56:51)
> Hi
> On Tue, Aug 28, 2018 at 2:05 AM Michael Roth <mdroth@linux.vnet.ibm.com> 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 <mdroth@linux.vnet.ibm.com>
> > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > > ---
> > > 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?
Looks good! With that:
Reviewed-by: Michael Roth <mdroth@linux.vnet.ibm.com>
>
> > > -}
> > > -
> > > /* 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
>
© 2016 - 2025 Red Hat, Inc.