Hi. I like your approach to list the command that is not found, however
I think the logic to detect an unknown command on the text monitor
should be broken out into a separate function (for reuse and
maintainability). I will respin a patch and let you decide if you agree.
On 08/22/2011 08:52 AM, Osier Yang wrote:
> * src/qemu/qemu_monitor_json.c: Handle error "CommandNotFound" and
> report the error.
>
> * src/qemu/qemu_monitor_text.c: If a sub info command is not found,
> it prints the output of "help info", for other commands,
> "unknown command" is printed.
>
> Without this patch, libvirt always report:
>
> An error occurred, but the cause is unknown
>
> ---
> src/qemu/qemu_monitor_json.c | 34 ++++++++++++++++++-----------
> src/qemu/qemu_monitor_text.c | 47 +++++++++++++++++++++++++++++++++---------
> 2 files changed, 58 insertions(+), 23 deletions(-)
>
> diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
> index 7adfb26..715b26e 100644
> --- a/src/qemu/qemu_monitor_json.c
> +++ b/src/qemu/qemu_monitor_json.c
> @@ -2909,20 +2909,25 @@ int qemuMonitorJSONBlockJob(qemuMonitorPtr mon,
> int ret = -1;
> virJSONValuePtr cmd = NULL;
> virJSONValuePtr reply = NULL;
> -
> - if (mode == BLOCK_JOB_ABORT)
> - cmd = qemuMonitorJSONMakeCommand("block_job_cancel",
> - "s:device", device, NULL);
> - else if (mode == BLOCK_JOB_INFO)
> - cmd = qemuMonitorJSONMakeCommand("query-block-jobs", NULL);
> - else if (mode == BLOCK_JOB_SPEED)
> - cmd = qemuMonitorJSONMakeCommand("block_job_set_speed",
> - "s:device", device,
> - "U:value", bandwidth * 1024ULL * 1024ULL,
> + const char *cmd_name = NULL;
> +
> + if (mode == BLOCK_JOB_ABORT) {
> + cmd_name = "block_job_cancel";
> + cmd = qemuMonitorJSONMakeCommand(cmd_name, "s:device", device, NULL);
> + } else if (mode == BLOCK_JOB_INFO) {
> + cmd_name = "query-block-jobs";
> + cmd = qemuMonitorJSONMakeCommand(cmd_name, NULL);
> + } else if (mode == BLOCK_JOB_SPEED) {
> + cmd_name = "block_job_set_speed";
> + cmd = qemuMonitorJSONMakeCommand(cmd_name, "s:device",
> + device, "U:value",
> + bandwidth * 1024ULL * 1024ULL,
> NULL);
> - else if (mode == BLOCK_JOB_PULL)
> - cmd = qemuMonitorJSONMakeCommand("block_stream",
> - "s:device", device, NULL);
> + } else if (mode == BLOCK_JOB_PULL) {
> + cmd_name = "block_stream";
> + cmd = qemuMonitorJSONMakeCommand(cmd_name, "s:device",
> + device, NULL);
> + }
>
> if (!cmd)
> return -1;
> @@ -2939,6 +2944,9 @@ int qemuMonitorJSONBlockJob(qemuMonitorPtr mon,
> else if (qemuMonitorJSONHasError(reply, "NotSupported"))
> qemuReportError(VIR_ERR_OPERATION_INVALID,
> _("Operation is not supported for device: %s"), device);
> + else if (qemuMonitorJSONHasError(reply, "CommandNotFound"))
> + qemuReportError(VIR_ERR_OPERATION_INVALID,
> + _("Command '%s' is not found"), cmd_name);
> else
> qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> _("Unexpected error"));
> diff --git a/src/qemu/qemu_monitor_text.c b/src/qemu/qemu_monitor_text.c
> index 52d924a..d296675 100644
> --- a/src/qemu/qemu_monitor_text.c
> +++ b/src/qemu/qemu_monitor_text.c
> @@ -3031,18 +3031,24 @@ int qemuMonitorTextBlockJob(qemuMonitorPtr mon,
> char *cmd = NULL;
> char *reply = NULL;
> int ret;
> -
> - if (mode == BLOCK_JOB_ABORT)
> - ret = virAsprintf(&cmd, "block_job_cancel %s", device);
> - else if (mode == BLOCK_JOB_INFO)
> - ret = virAsprintf(&cmd, "info block-jobs");
> - else if (mode == BLOCK_JOB_SPEED)
> - ret = virAsprintf(&cmd, "block_job_set_speed %s %llu", device,
> + const char *cmd_name = NULL;
> +
> + if (mode == BLOCK_JOB_ABORT) {
> + cmd_name = "block_job_cancel";
> + ret = virAsprintf(&cmd, "%s %s", cmd_name, device);
> + } else if (mode == BLOCK_JOB_INFO) {
> + cmd_name = "info block-jobs";
> + ret = virAsprintf(&cmd, "%s", cmd_name);
> + } else if (mode == BLOCK_JOB_SPEED) {
> + cmd_name = "block_job_set_speed";
> + ret = virAsprintf(&cmd, "%s %s %llu", cmd_name, device,
> bandwidth * 1024ULL * 1024ULL);
> - else if (mode == BLOCK_JOB_PULL)
> - ret = virAsprintf(&cmd, "block_stream %s", device);
> - else
> + } else if (mode == BLOCK_JOB_PULL) {
> + cmd_name = "block_stream";
> + ret = virAsprintf(&cmd, "%s %s", cmd_name, device);
> + } else {
> return -1;
> + }
>
> if (ret < 0) {
> virReportOOMError();
> @@ -3056,6 +3062,27 @@ int qemuMonitorTextBlockJob(qemuMonitorPtr mon,
> goto cleanup;
> }
>
> + /* Check error: Command not found, for "info block-jobs" command.
> + * If qemu monitor command "info" doesn't support one sub-command,
> + * it will print the output of "help info" instead, so simply check
> + * if "info version" is in the output.
> + */
> + if (mode == BLOCK_JOB_INFO) {
> + if (strstr(reply, "info version")) {
> + qemuReportError(VIR_ERR_OPERATION_INVALID,
> + _("Command '%s' is not found"), cmd_name);
> + ret = -1;
> + goto cleanup;
> + }
> + } else {
> + if (strstr(reply, "unknown command")) {
> + qemuReportError(VIR_ERR_OPERATION_INVALID,
> + _("Command '%s' is not found"), cmd_name);
> + ret = -1;
> + goto cleanup;
> + }
> + }
> +
> ret = qemuMonitorTextParseBlockJob(reply, device, info);
>
> cleanup:
--
Adam Litke
IBM Linux Technology Center
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list