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

[Markus Armbruster]

Bandan Das <address@hidden> writes:

> Markus Armbruster <address@hidden> writes:
>
>> Bandan Das <address@hidden> 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.
> ...

I think limiting the additional help to argument syntax errors for now
will be easiest.

>>>  
>>>  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.

Looking forward to it :)

>>>  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.

You're right.  I got confused, figured out what's up, updated my review,
but missed this paragraph.

[...]