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

[Osier Yang <jyang@xxxxxxxxxx>]

于 2011年08月23日 00:53, Adam Litke 写道:
> From: Osier Yang <jyang@xxxxxxxxxx>
>
> * 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
>
> This patch was adapted from a patch by Osier Yang <jyang@xxxxxxxxxx> to break
> out detection of unrecognized text monitor commands into a separate function.
>
> Signed-off-by: Adam Litke <agl@xxxxxxxxxx>
> ---
>  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..9119f06 100644
> --- a/src/qemu/qemu_monitor_text.c
> +++ b/src/qemu/qemu_monitor_text.c
> @@ -271,6 +271,20 @@ qemuMonitorTextCommandWithFd(qemuMonitorPtr mon,
>                                               scm_fd, reply);
>  }
>  
> +/* Check monitor output for evidence that the command was not recognized.
> + * For 'info' commands, qemu returns help text.  For other commands, qemu
> + * returns 'unknown command:'.
> + */
> +static int
> +qemuMonitorTextCommandNotFound(const char *reply)
> +{
> +    if (strstr(reply, "unknown command:"))
> +        return 1;
> +    if (strstr(reply, "info version"))
> +        return 1;
> +    return 0;
> +}
> +

Hi, Adam

I likes the idea to wrap the checking as a function seperately, but the
function
won't work well if the command is "help info", though we don't have a use
of "help info" yet.

My point is since the function is intending to work for all the command,
as a
common function, it needs to consider some boudary too.

How about below?

qemuMonitorTextCommandNotFound(const char *cmd, const char *reply)
{
    if (STRPREFIX(cmd, "info ")) {
        if (strstr(reply, "info version"))
            return 1;
    } else {
        if (strstr(reply, "unknown command:"))
            return 1;
    }

    return 0;
}

And there might be other different info qemu will output for a unknown
command
we don't known yet. Using "cmd" as an argument will allow us to extend
the checking
methods.

Thanks
Osier

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list