Re: [Qemu-devel] [PATCH] monitor: suggest running "help" for command errors

[Bandan Das <bsd@redhat.com>]

Markus Armbruster <armbru@redhat.com> writes:

> Bandan Das <bsd@redhat.com> writes:
>
...
>> -static void user_async_cmd_handler(Monitor *mon, const mon_cmd_t *cmd,
>> +static int user_async_cmd_handler(Monitor *mon, const mon_cmd_t *cmd,
>>                                     const QDict *params)
>>  {
>>      int ret;
>> @@ -954,6 +954,8 @@ static void user_async_cmd_handler(Monitor *mon, const mon_cmd_t *cmd,
>>          monitor_resume(mon);
>>          g_free(cb_data);
>>      }
>> +
>> +    return ret;
>>  }
>>  
>
> user_async_cmd_handler() is unreachable since commit 3b5704b.  I got
> code cleaning that up in my tree.  Don't worry about it.

Ok or if you prefer, I can skip this part and other similar cases below.
...
>>  
>>  fail:
>> +    *failed = 1;
>>      g_free(key);
>> -    return NULL;
>> +    return cmd;
>>  }
>>  
>
> From the function's contract:
>
>  * If @cmdline is blank, return NULL.
>  * If it can't be parsed, report to @mon, and return NULL.
>  * Else, insert command arguments into @qdict, and return the command.
>
> Your patch splits the "it can't be parsed" case into "command not found"
> and "arguments can't be parsed", but neglects to update the contract.
> Needs fixing.  Perhaps like this:
>
>  * If @cmdline is blank, return NULL.
>  * If @cmdline doesn't start with a valid command name, report to @mon,
>  * and return NULL.
>  * Else, if the command's arguments can't be parsed, report to @mon,
>  * store 1 through @failed, and return the command.
>  * Else, insert command arguments into @qdict, and return the command.


> The contract wasn't exactly pretty before, and I'm afraid it's
> positively ugly now.
>
> The common cause for such ugliness is doing too much in one function.
> I'd try splitting into a command part and an argument part, so that
>
>     cmd = monitor_parse_command(mon, cmdline, start, table, qdict);
>     if (!cmd) {
>         // bail out
>     }
>
> becomes something like
>
>     cmd = monitor_parse_command(mon, cmdline, start, table, &args);
>     if (!cmd) {
>         // bail out
>     }
>     qdict = monitor_parse_arguments(mon, cmd, args)
>     if (!qdict) {
>         // bail out
>     }
>
> While I encourage you to do this, I won't reject your patch just because
> you don't.

Ok understood, makes sense. Will fix this in next version.

>>  void monitor_set_error(Monitor *mon, QError *qerror)
>> @@ -4114,20 +4118,22 @@ static void handle_user_command(Monitor *mon, const char *cmdline)
>>  {
>>      QDict *qdict;
>>      const mon_cmd_t *cmd;
>> +    int failed = 0;
>>  
>>      qdict = qdict_new();
>>  
>> -    cmd = monitor_parse_command(mon, cmdline, 0, mon->cmd_table, qdict);
>> -    if (!cmd)
>> +    cmd = monitor_parse_command(mon, cmdline, 0, mon->cmd_table,
>> +                                qdict, &failed);
>> +    if (!cmd || failed) {
>>          goto out;
>> +    }
>
> cmd implies !failed, therefore !cmd || failed == !cmd here.  Why not
> simply stick to if (!cmd)?

Note that I changed the old behavior so that now monitor_parse_command()
returns cmd when failed is set. This is so that we have cmd->name. "if (!cmd)"
will be false in that case.

>>  
>>      if (handler_is_async(cmd)) {
>> -        user_async_cmd_handler(mon, cmd, qdict);
>> +        failed = user_async_cmd_handler(mon, cmd, qdict);
>
> As discussed above: unreachable, but don't worry about it.
>
>>      } else if (handler_is_qobject(cmd)) {
>
> This is the "new" HMP command interface.  It's used only with drive_del,
> device_add, client_migrate_info, pcie_aer_inject_error.  The cleanup
> work I mentioned above will get rid of it.  Again, don't worry.

Ok.

>>          QObject *data = NULL;
>>  
>> -        /* XXX: ignores the error code */
>> -        cmd->mhandler.cmd_new(mon, qdict, &data);
>> +        failed = cmd->mhandler.cmd_new(mon, qdict, &data);
>>          assert(!monitor_has_error(mon));
>>          if (data) {
>>              cmd->user_print(mon, data);
>                qobject_decref(data);
>            }
>        } else {
>
> This is the traditional HMP command interface.  Almost all commands use
> it, and after my pending cleanup, it'll be the *only* HMP command
> interface.
>
> It lacks means to communicate command failure.
>
>            cmd->mhandler.cmd(mon, qdict);
>        }
>
> Since you can't set failed in the common case, I recommend not to bother
> setting it in the unreachable and the uncommon case, either.
>
>> @@ -4138,6 +4144,10 @@ static void handle_user_command(Monitor *mon, const char *cmdline)
>>      }
>>  
>>  out:
>> +    if (failed && cmd) {
>
> Recommend (cmd && failed).

Since this path is common whether the command failed or not, I
think it's better to check for "failed" as the first condition.
So, the check on second argument need not be done if the function
succeeds. Actually, taking a second look, I think just
"if (failed) {" should be enough since cmd is guaranteed to be !NULL
when failed is set.

Bandan

>> +        monitor_printf(mon, "Try \"help %s\" for more information\n",
>> +                       cmd->name);
>> +    }
>>      QDECREF(qdict);
>>  }
>
> Cases:
>
> 1. @cmdline is blank
>
>    cmd == NULL && !failed
>
>    We don't print command help.  Good.
>
> 2. @cmdline isn't blank, command not found
>
>    cmd == NULL && !failed
>
>    We don't print command help.  We already printed the error.  Good.
>
> 3. command found, arguments can't be parsed
>
>    cmd != NULL && failed
>
>    We print command help.  We printed the parse error already.  Good.
>
> 4. command found, arguments parsed, command failed
>
>    cmd != NULL, but failed can be anything
>
>    We may or may not print command help.  The command should've printed
>    an error already.  Best we can do as long as the traditional HMP
>    command handler doesn't return status.
>
> 5. command found, arguments parsed, command succeeded
>
>    We don't print command help.  Good.
>
> Works.