From 46abb8124006887d071921c5e657eeec3c50a9e2 Mon Sep 17 00:00:00 2001 From: Luiz Capitulino Date: Tue, 31 Mar 2015 13:00:26 -0400 Subject: [PATCH 1/4] balloon: improve error msg when adding second device A VM supports only one balloon device, but due to several changes in infrastructure the error message got messed up when trying to add a second device. Fix it. Before this fix Command-line: qemu-qmp: -device virtio-balloon-pci,id=balloon0: Another balloon device already registered qemu-qmp: -device virtio-balloon-pci,id=balloon0: Adding balloon handler failed qemu-qmp: -device virtio-balloon-pci,id=balloon0: Device 'virtio-balloon-pci' could not be initialized HMP: Another balloon device already registered Adding balloon handler failed Device 'virtio-balloon-pci' could not be initialized QMP: { "execute": "device_add", "arguments": { "driver": "virtio-balloon-pci", "id": "balloon0" } } { "error": { "class": "GenericError", "desc": "Adding balloon handler failed" } } After this fix Command-line: qemu-qmp: -device virtio-balloon-pci,id=balloon0: Only one balloon device is supported qemu-qmp: -device virtio-balloon-pci,id=balloon0: Device 'virtio-balloon-pci' could not be initialized HMP: (qemu) device_add virtio-balloon-pci,id=balloon0 Only one balloon device is supported Device 'virtio-balloon-pci' could not be initialized (qemu) QMP: { "execute": "device_add", "arguments": { "driver": "virtio-balloon-pci", "id": "balloon0" } } { "error": { "class": "GenericError", "desc": "Only one balloon device is supported" } } Signed-off-by: Luiz Capitulino Reviewed-by: Eric Blake --- balloon.c | 1 - hw/virtio/virtio-balloon.c | 2 +- 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/balloon.c b/balloon.c index 70c00f5f84..c7033e3dc3 100644 --- a/balloon.c +++ b/balloon.c @@ -58,7 +58,6 @@ int qemu_add_balloon_handler(QEMUBalloonEvent *event_func, /* We're already registered one balloon handler. How many can * a guest really have? */ - error_report("Another balloon device already registered"); return -1; } balloon_event_fn = event_func; diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c index 95b0643448..484c3c333c 100644 --- a/hw/virtio/virtio-balloon.c +++ b/hw/virtio/virtio-balloon.c @@ -383,7 +383,7 @@ static void virtio_balloon_device_realize(DeviceState *dev, Error **errp) virtio_balloon_stat, s); if (ret < 0) { - error_setg(errp, "Adding balloon handler failed"); + error_setg(errp, "Only one balloon device is supported"); virtio_cleanup(vdev); return; } From 6540e9f35bfeea2baf4509745516172070dca412 Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Fri, 10 Apr 2015 15:07:59 -0600 Subject: [PATCH 2/4] qapi: Drop dead genlist parameter Defaulting a parameter to True, then having all callers omit or pass an explicit True for that parameter, is pointless. Looks like it has been dead since introduction in commit 06d64c6, more than 4 years ago. Signed-off-by: Eric Blake Signed-off-by: Luiz Capitulino --- scripts/qapi-visit.py | 22 +++++++++------------- 1 file changed, 9 insertions(+), 13 deletions(-) diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py index 8f845a2b29..1be4d67d8a 100644 --- a/scripts/qapi-visit.py +++ b/scripts/qapi-visit.py @@ -2,7 +2,7 @@ # QAPI visitor generator # # Copyright IBM, Corp. 2011 -# Copyright (C) 2014 Red Hat, Inc. +# Copyright (C) 2014-2015 Red Hat, Inc. # # Authors: # Anthony Liguori @@ -401,34 +401,31 @@ out: return ret -def generate_declaration(name, members, genlist=True, builtin_type=False): +def generate_declaration(name, members, builtin_type=False): ret = "" if not builtin_type: ret += mcgen(''' void visit_type_%(name)s(Visitor *m, %(name)s **obj, const char *name, Error **errp); ''', - name=name) + name=name) - if genlist: - ret += mcgen(''' + ret += mcgen(''' void visit_type_%(name)sList(Visitor *m, %(name)sList **obj, const char *name, Error **errp); ''', name=name) return ret -def generate_enum_declaration(name, members, genlist=True): - ret = "" - if genlist: - ret += mcgen(''' +def generate_enum_declaration(name, members): + ret = mcgen(''' void visit_type_%(name)sList(Visitor *m, %(name)sList **obj, const char *name, Error **errp); ''', - name=name) + name=name) return ret -def generate_decl_enum(name, members, genlist=True): +def generate_decl_enum(name, members): return mcgen(''' void visit_type_%(name)s(Visitor *m, %(name)s *obj, const char *name, Error **errp); @@ -542,8 +539,7 @@ exprs = parse_schema(input_file) # for built-in types in our header files and simply guard them fdecl.write(guardstart("QAPI_VISIT_BUILTIN_VISITOR_DECL")) for typename in builtin_types: - fdecl.write(generate_declaration(typename, None, genlist=True, - builtin_type=True)) + fdecl.write(generate_declaration(typename, None, builtin_type=True)) fdecl.write(guardend("QAPI_VISIT_BUILTIN_VISITOR_DECL")) # ...this doesn't work for cases where we link in multiple objects that From 43d0a2c1af5bc77ed067636db956209779351dfe Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Wed, 15 Apr 2015 13:30:04 +0200 Subject: [PATCH 3/4] qmp-commands: fix incorrect uses of ":O" specifier As far as the QMP parser is concerned, neither the 'O' nor the 'q' format specifiers put any constraint on the command. However, there are two differences: 1) from a documentation point of view 'O' says that this command takes a dictionary. The dictionary will be converted to QemuOpts in the handler to match the corresponding HMP command. 2) 'O' sets QMP_ACCEPT_UNKNOWNS, resulting in the command accepting invalid extra arguments. For example the following is accepted: { "execute": "send-key", "arguments": { "keys": [ { "type": "qcode", "data": "ctrl" }, { "type": "qcode", "data": "alt" }, { "type": "qcode", "data": "delete" } ], "foo": "bar" } } Neither send-key nor migrate-set-capabilities take a QemuOpts-like dictionary; they take an array of dictionaries. And neither command really wants to have extra unknown arguments. Thus, the right specifier to use in this case is 'q'; with this patch the above command fails with {"error": {"class": "GenericError", "desc": "Invalid parameter 'foo'"}} as intended. Reported-by: Alberto Garcia Signed-off-by: Paolo Bonzini Reviewed-by: Alberto Garcia Signed-off-by: Luiz Capitulino --- qmp-commands.hx | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/qmp-commands.hx b/qmp-commands.hx index 3a42ad0bff..09f48bada5 100644 --- a/qmp-commands.hx +++ b/qmp-commands.hx @@ -332,7 +332,7 @@ EQMP { .name = "send-key", - .args_type = "keys:O,hold-time:i?", + .args_type = "keys:q,hold-time:i?", .mhandler.cmd_new = qmp_marshal_input_send_key, }, @@ -3288,7 +3288,7 @@ EQMP { .name = "migrate-set-capabilities", - .args_type = "capabilities:O", + .args_type = "capabilities:q", .params = "capability:s,state:b", .mhandler.cmd_new = qmp_marshal_input_migrate_set_capabilities, }, From 2d5a8346a484250934526a15b3a522bdba7e6772 Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Wed, 15 Apr 2015 09:19:23 -0600 Subject: [PATCH 4/4] qmp: Give saner messages related to qmp_capabilities misuse Pretending that QMP doesn't understand a command merely because we are not in the right mode doesn't help first-time users figure out what to do to correct things. Although the documentation for QMP calls out capabilities negotiation, we should also make it clear in our error messages what we were expecting. With this patch, I now get the following transcript: $ ./x86_64-softmmu/qemu-system-x86_64 -qmp stdio -nodefaults {"QMP": {"version": {"qemu": {"micro": 93, "minor": 2, "major": 2}, "package": ""}, "capabilities": []}} {"execute":"huh"} {"error": {"class": "CommandNotFound", "desc": "The command huh has not been found"}} {"execute":"quit"} {"error": {"class": "CommandNotFound", "desc": "Expecting capabilities negotiation with 'qmp_capabilities' before command 'quit'"}} {"execute":"qmp_capabilities"} {"return": {}} {"execute":"qmp_capabilities"} {"error": {"class": "CommandNotFound", "desc": "Capabilities negotiation is already complete, command 'qmp_capabilities' ignored"}} {"execute":"quit"} {"return": {}} {"timestamp": {"seconds": 1429110729, "microseconds": 181935}, "event": "SHUTDOWN"} Signed-off-by: Eric Blake Tested-By: Kashyap Chamarthy Reviewed-by: Paulo Vital Reviewed-by: John Snow Signed-off-by: Luiz Capitulino --- monitor.c | 23 +++++++++++++++++++---- 1 file changed, 19 insertions(+), 4 deletions(-) diff --git a/monitor.c b/monitor.c index 68873ec09c..9f37700486 100644 --- a/monitor.c +++ b/monitor.c @@ -4783,10 +4783,22 @@ static int monitor_can_read(void *opaque) return (mon->suspend_cnt == 0) ? 1 : 0; } -static int invalid_qmp_mode(const Monitor *mon, const mon_cmd_t *cmd) +static bool invalid_qmp_mode(const Monitor *mon, const mon_cmd_t *cmd) { - int is_cap = cmd->mhandler.cmd_new == do_qmp_capabilities; - return (qmp_cmd_mode(mon) ? is_cap : !is_cap); + bool is_cap = cmd->mhandler.cmd_new == do_qmp_capabilities; + if (is_cap && qmp_cmd_mode(mon)) { + qerror_report(ERROR_CLASS_COMMAND_NOT_FOUND, + "Capabilities negotiation is already complete, command " + "'%s' ignored", cmd->name); + return true; + } + if (!is_cap && !qmp_cmd_mode(mon)) { + qerror_report(ERROR_CLASS_COMMAND_NOT_FOUND, + "Expecting capabilities negotiation with " + "'qmp_capabilities' before command '%s'", cmd->name); + return true; + } + return false; } /* @@ -5080,11 +5092,14 @@ static void handle_qmp_command(JSONMessageParser *parser, QList *tokens) cmd_name = qdict_get_str(input, "execute"); trace_handle_qmp_command(mon, cmd_name); cmd = qmp_find_cmd(cmd_name); - if (!cmd || invalid_qmp_mode(mon, cmd)) { + if (!cmd) { qerror_report(ERROR_CLASS_COMMAND_NOT_FOUND, "The command %s has not been found", cmd_name); goto err_out; } + if (invalid_qmp_mode(mon, cmd)) { + goto err_out; + } obj = qdict_get(input, "arguments"); if (!obj) {