* [Qemu-devel] [PATCH 1/2] qga: process_event() simplification
2019-02-20 15:42 [Qemu-devel] [PATCH 0/2] Misc QGA/monitor QMP improvements (resend) Marc-André Lureau
@ 2019-02-20 15:42 ` Marc-André Lureau
2019-02-20 16:21 ` Eric Blake
2019-02-20 15:42 ` [Qemu-devel] [PATCH 2/2] qmp: common 'id' handling & make QGA conform to QMP spec Marc-André Lureau
2019-02-27 7:09 ` [Qemu-devel] [PATCH 0/2] Misc QGA/monitor QMP improvements (resend) Markus Armbruster
2 siblings, 1 reply; 5+ messages in thread
From: Marc-André Lureau @ 2019-02-20 15:42 UTC (permalink / raw)
To: qemu-devel
Cc: Dr. David Alan Gilbert, Michael Roth, Markus Armbruster,
Marc-André Lureau
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 87a0711c14..c0d77c79c4 100644
--- a/qga/main.c
+++ b/qga/main.c
@@ -523,15 +523,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;
}
@@ -557,53 +557,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.21.0.rc1
^ permalink raw reply related [flat|nested] 5+ messages in thread* [Qemu-devel] [PATCH 2/2] qmp: common 'id' handling & make QGA conform to QMP spec
2019-02-20 15:42 [Qemu-devel] [PATCH 0/2] Misc QGA/monitor QMP improvements (resend) Marc-André Lureau
2019-02-20 15:42 ` [Qemu-devel] [PATCH 1/2] qga: process_event() simplification Marc-André Lureau
@ 2019-02-20 15:42 ` Marc-André Lureau
2019-02-27 7:09 ` [Qemu-devel] [PATCH 0/2] Misc QGA/monitor QMP improvements (resend) Markus Armbruster
2 siblings, 0 replies; 5+ messages in thread
From: Marc-André Lureau @ 2019-02-20 15:42 UTC (permalink / raw)
To: qemu-devel
Cc: Dr. David Alan Gilbert, Michael Roth, Markus Armbruster,
Marc-André Lureau
Let qmp_dispatch() copy the 'id' field. That way any qmp client will
conform to the specification, including QGA. Furthermore, it
simplifies the work for qemu monitor.
CC: Michael Roth <mdroth@linux.vnet.ibm.com>
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
---
monitor.c | 33 ++++++++++++---------------------
qapi/qmp-dispatch.c | 10 ++++++++--
tests/test-qga.c | 13 +++++--------
3 files changed, 25 insertions(+), 31 deletions(-)
diff --git a/monitor.c b/monitor.c
index 33ccbf3957..c0bc089645 100644
--- a/monitor.c
+++ b/monitor.c
@@ -249,8 +249,6 @@ QEMUBH *qmp_dispatcher_bh;
struct QMPRequest {
/* Owner of the request */
Monitor *mon;
- /* "id" field of the request */
- QObject *id;
/*
* Request object to be handled or Error to be reported
* (exactly one of them is non-null)
@@ -352,7 +350,6 @@ int monitor_read_password(Monitor *mon, ReadLineFunc *readline_func,
static void qmp_request_free(QMPRequest *req)
{
- qobject_unref(req->id);
qobject_unref(req->req);
error_free(req->err);
g_free(req);
@@ -4026,18 +4023,14 @@ static int monitor_can_read(void *opaque)
* Null @rsp can only happen for commands with QCO_NO_SUCCESS_RESP.
* Nothing is emitted then.
*/
-static void monitor_qmp_respond(Monitor *mon, QDict *rsp, QObject *id)
+static void monitor_qmp_respond(Monitor *mon, QDict *rsp)
{
if (rsp) {
- if (id) {
- qdict_put_obj(rsp, "id", qobject_ref(id));
- }
-
qmp_send_response(mon, rsp);
}
}
-static void monitor_qmp_dispatch(Monitor *mon, QObject *req, QObject *id)
+static void monitor_qmp_dispatch(Monitor *mon, QObject *req)
{
Monitor *old_mon;
QDict *rsp;
@@ -4062,7 +4055,7 @@ static void monitor_qmp_dispatch(Monitor *mon, QObject *req, QObject *id)
}
}
- monitor_qmp_respond(mon, rsp, id);
+ monitor_qmp_respond(mon, rsp);
qobject_unref(rsp);
}
@@ -4126,13 +4119,15 @@ static void monitor_qmp_bh_dispatcher(void *data)
mon->qmp.qmp_requests->length == QMP_REQ_QUEUE_LEN_MAX - 1;
qemu_mutex_unlock(&mon->qmp.qmp_queue_lock);
if (req_obj->req) {
- trace_monitor_qmp_cmd_in_band(qobject_get_try_str(req_obj->id) ?: "");
- monitor_qmp_dispatch(mon, req_obj->req, req_obj->id);
+ QDict *qdict = qobject_to(QDict, req_obj->req);
+ QObject *id = qdict ? qdict_get(qdict, "id") : NULL;
+ trace_monitor_qmp_cmd_in_band(qobject_get_try_str(id) ?: "");
+ monitor_qmp_dispatch(mon, req_obj->req);
} else {
assert(req_obj->err);
rsp = qmp_error_response(req_obj->err);
req_obj->err = NULL;
- monitor_qmp_respond(mon, rsp, NULL);
+ monitor_qmp_respond(mon, rsp);
qobject_unref(rsp);
}
@@ -4157,8 +4152,7 @@ static void handle_qmp_command(void *opaque, QObject *req, Error *err)
qdict = qobject_to(QDict, req);
if (qdict) {
- id = qobject_ref(qdict_get(qdict, "id"));
- qdict_del(qdict, "id");
+ id = qdict_get(qdict, "id");
} /* else will fail qmp_dispatch() */
if (req && trace_event_get_state_backends(TRACE_HANDLE_QMP_COMMAND)) {
@@ -4169,17 +4163,14 @@ static void handle_qmp_command(void *opaque, QObject *req, Error *err)
if (qdict && qmp_is_oob(qdict)) {
/* OOB commands are executed immediately */
- trace_monitor_qmp_cmd_out_of_band(qobject_get_try_str(id)
- ?: "");
- monitor_qmp_dispatch(mon, req, id);
+ trace_monitor_qmp_cmd_out_of_band(qobject_get_try_str(id) ?: "");
+ monitor_qmp_dispatch(mon, req);
qobject_unref(req);
- qobject_unref(id);
return;
}
req_obj = g_new0(QMPRequest, 1);
req_obj->mon = mon;
- req_obj->id = id;
req_obj->req = req;
req_obj->err = err;
@@ -4199,7 +4190,7 @@ static void handle_qmp_command(void *opaque, QObject *req, Error *err)
/*
* Put the request to the end of queue so that requests will be
- * handled in time order. Ownership for req_obj, req, id,
+ * handled in time order. Ownership for req_obj, req,
* etc. will be delivered to the handler side.
*/
assert(mon->qmp.qmp_requests->length < QMP_REQ_QUEUE_LEN_MAX);
diff --git a/qapi/qmp-dispatch.c b/qapi/qmp-dispatch.c
index 1d922e04f7..5f812bb9f2 100644
--- a/qapi/qmp-dispatch.c
+++ b/qapi/qmp-dispatch.c
@@ -58,6 +58,8 @@ static QDict *qmp_dispatch_check_obj(const QObject *request, bool allow_oob,
"QMP input member 'arguments' must be an object");
return NULL;
}
+ } else if (!strcmp(arg_name, "id")) {
+ continue;
} else {
error_setg(errp, "QMP input member '%s' is unexpected",
arg_name);
@@ -165,11 +167,11 @@ QDict *qmp_dispatch(QmpCommandList *cmds, QObject *request,
bool allow_oob)
{
Error *err = NULL;
- QObject *ret;
+ QDict *dict = qobject_to(QDict, request);
+ QObject *ret, *id = dict ? qdict_get(dict, "id") : NULL;
QDict *rsp;
ret = do_qmp_dispatch(cmds, request, allow_oob, &err);
-
if (err) {
rsp = qmp_error_response(err);
} else if (ret) {
@@ -180,5 +182,9 @@ QDict *qmp_dispatch(QmpCommandList *cmds, QObject *request,
rsp = NULL;
}
+ if (rsp && id) {
+ qdict_put_obj(rsp, "id", qobject_ref(id));
+ }
+
return rsp;
}
diff --git a/tests/test-qga.c b/tests/test-qga.c
index 3d6377436a..891aa3d322 100644
--- a/tests/test-qga.c
+++ b/tests/test-qga.c
@@ -225,18 +225,15 @@ static void test_qga_ping(gconstpointer fix)
qobject_unref(ret);
}
-static void test_qga_invalid_id(gconstpointer fix)
+static void test_qga_id(gconstpointer fix)
{
const TestFixture *fixture = fix;
- QDict *ret, *error;
- const char *class;
+ QDict *ret;
ret = qmp_fd(fixture->fd, "{'execute': 'guest-ping', 'id': 1}");
g_assert_nonnull(ret);
-
- error = qdict_get_qdict(ret, "error");
- class = qdict_get_try_str(error, "class");
- g_assert_cmpstr(class, ==, "GenericError");
+ qmp_assert_no_error(ret);
+ g_assert_cmpint(qdict_get_int(ret, "id"), ==, 1);
qobject_unref(ret);
}
@@ -992,7 +989,7 @@ int main(int argc, char **argv)
g_test_add_data_func("/qga/file-ops", &fix, test_qga_file_ops);
g_test_add_data_func("/qga/file-write-read", &fix, test_qga_file_write_read);
g_test_add_data_func("/qga/get-time", &fix, test_qga_get_time);
- g_test_add_data_func("/qga/invalid-id", &fix, test_qga_invalid_id);
+ g_test_add_data_func("/qga/id", &fix, test_qga_id);
g_test_add_data_func("/qga/invalid-oob", &fix, test_qga_invalid_oob);
g_test_add_data_func("/qga/invalid-cmd", &fix, test_qga_invalid_cmd);
g_test_add_data_func("/qga/invalid-args", &fix, test_qga_invalid_args);
--
2.21.0.rc1
^ permalink raw reply related [flat|nested] 5+ messages in thread