Markus Armbruster <arm...@redhat.com> writes:
> Bandan Das <b...@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.
