Bandan Das <b...@redhat.com> writes: > 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. > ...
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. [...]