[libvirt] [PATCH] qemu: Report error if qemu monitor command not found for BlockJob

[Adam Litke]

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