diff mbox

[02/20] monitor: Clean up after previous commit

Message ID 1432294585-5984-3-git-send-email-armbru@redhat.com
State New
Headers show

Commit Message

Markus Armbruster May 22, 2015, 11:36 a.m. UTC
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 monitor.c | 40 +++++++++++-----------------------------
 1 file changed, 11 insertions(+), 29 deletions(-)

Comments

Eric Blake May 22, 2015, 9:19 p.m. UTC | #1
On 05/22/2015 05:36 AM, Markus Armbruster wrote:
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---

Commit message is sparse; I would have mentioned [1] and [2].

>  monitor.c | 40 +++++++++++-----------------------------
>  1 file changed, 11 insertions(+), 29 deletions(-)
> 
>  
> -static void handler_audit(Monitor *mon, const mon_cmd_t *cmd, int ret)
> -{
> -    if (ret && !monitor_has_error(mon)) {
> -        /*
> -         * If it returns failure, it must have passed on error.
> -         *
> -         * Action: Report an internal error to the client if in QMP.
> -         */
> -        qerror_report(QERR_UNDEFINED_ERROR);
> -    }
> -}

[2] - handler_audit() goes away now that it has no caller

> -
>  static void handle_user_command(Monitor *mon, const char *cmdline)
>  {
>      QDict *qdict;
> @@ -5015,28 +5003,17 @@ static QDict *qmp_check_input_obj(QObject *input_obj)
>      return input_dict;
>  }
>  
> -static void qmp_call_cmd(Monitor *mon, const mon_cmd_t *cmd,
> -                         const QDict *params)
> -{
> -    int ret;
> -    QObject *data = NULL;
> -
> -    ret = cmd->mhandler.cmd_new(mon, params, &data);
> -    handler_audit(mon, cmd, ret);
> -    monitor_protocol_emitter(mon, data);
> -    qobject_decref(data);
> -}

[1] - qmp_call_cmd() is inlined and simplified...

> -
>  static void handle_qmp_command(JSONMessageParser *parser, QList *tokens)
>  {
>      int err;
> -    QObject *obj;
> +    QObject *obj, *data;
>      QDict *input, *args;
>      const mon_cmd_t *cmd;
>      const char *cmd_name;
>      Monitor *mon = cur_mon;
>  
>      args = input = NULL;
> +    data = NULL;
>  
>      obj = json_parser_parse(tokens, NULL);
>      if (!obj) {
> @@ -5079,12 +5056,17 @@ static void handle_qmp_command(JSONMessageParser *parser, QList *tokens)
>          goto err_out;
>      }
>  
> -    qmp_call_cmd(mon, cmd, args);
> -    goto out;
> +    if (cmd->mhandler.cmd_new(mon, args, &data)) {
> +        /* Command failed... */
> +        if (!monitor_has_error(mon)) {
> +            /* ... without setting an error, so make one up */
> +            qerror_report(QERR_UNDEFINED_ERROR);
> +        }
> +    }

...into its lone caller.

>  
>  err_out:
> -    monitor_protocol_emitter(mon, NULL);
> -out:
> +    monitor_protocol_emitter(mon, data);
> +    qobject_decref(data);
>      QDECREF(input);
>      QDECREF(args);
>  }
> 

But the code cleanup itself looks sane, so with the improved commit message:

Reviewed-by: Eric Blake <eblake@redhat.com>
Markus Armbruster May 26, 2015, 9:01 a.m. UTC | #2
Eric Blake <eblake@redhat.com> writes:

> On 05/22/2015 05:36 AM, Markus Armbruster wrote:
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>
> Commit message is sparse; I would have mentioned [1] and [2].
>
>>  monitor.c | 40 +++++++++++-----------------------------
>>  1 file changed, 11 insertions(+), 29 deletions(-)
>> 
>>  
>> -static void handler_audit(Monitor *mon, const mon_cmd_t *cmd, int ret)
>> -{
>> -    if (ret && !monitor_has_error(mon)) {
>> -        /*
>> -         * If it returns failure, it must have passed on error.
>> -         *
>> -         * Action: Report an internal error to the client if in QMP.
>> -         */
>> -        qerror_report(QERR_UNDEFINED_ERROR);
>> -    }
>> -}
>
> [2] - handler_audit() goes away now that it has no caller
>
>> -
>>  static void handle_user_command(Monitor *mon, const char *cmdline)
>>  {
>>      QDict *qdict;
>> @@ -5015,28 +5003,17 @@ static QDict *qmp_check_input_obj(QObject *input_obj)
>>      return input_dict;
>>  }
>>  
>> -static void qmp_call_cmd(Monitor *mon, const mon_cmd_t *cmd,
>> -                         const QDict *params)
>> -{
>> -    int ret;
>> -    QObject *data = NULL;
>> -
>> -    ret = cmd->mhandler.cmd_new(mon, params, &data);
>> -    handler_audit(mon, cmd, ret);
>> -    monitor_protocol_emitter(mon, data);
>> -    qobject_decref(data);
>> -}
>
> [1] - qmp_call_cmd() is inlined and simplified...
>
>> -
>>  static void handle_qmp_command(JSONMessageParser *parser, QList *tokens)
>>  {
>>      int err;
>> -    QObject *obj;
>> +    QObject *obj, *data;
>>      QDict *input, *args;
>>      const mon_cmd_t *cmd;
>>      const char *cmd_name;
>>      Monitor *mon = cur_mon;
>>  
>>      args = input = NULL;
>> +    data = NULL;
>>  
>>      obj = json_parser_parse(tokens, NULL);
>>      if (!obj) {
>> @@ -5079,12 +5056,17 @@ static void handle_qmp_command(JSONMessageParser *parser, QList *tokens)
>>          goto err_out;
>>      }
>>  
>> -    qmp_call_cmd(mon, cmd, args);
>> -    goto out;
>> +    if (cmd->mhandler.cmd_new(mon, args, &data)) {
>> +        /* Command failed... */
>> +        if (!monitor_has_error(mon)) {
>> +            /* ... without setting an error, so make one up */
>> +            qerror_report(QERR_UNDEFINED_ERROR);
>> +        }
>> +    }
>
> ...into its lone caller.

Can do.

>>  
>>  err_out:
>> -    monitor_protocol_emitter(mon, NULL);
>> -out:
>> +    monitor_protocol_emitter(mon, data);
>> +    qobject_decref(data);
>>      QDECREF(input);
>>      QDECREF(args);
>>  }
>> 
>
> But the code cleanup itself looks sane, so with the improved commit message:
>
> Reviewed-by: Eric Blake <eblake@redhat.com>

Thanks!
diff mbox

Patch

diff --git a/monitor.c b/monitor.c
index 9a0c3b7..5330e61 100644
--- a/monitor.c
+++ b/monitor.c
@@ -4045,18 +4045,6 @@  void monitor_set_error(Monitor *mon, QError *qerror)
     }
 }
 
-static void handler_audit(Monitor *mon, const mon_cmd_t *cmd, int ret)
-{
-    if (ret && !monitor_has_error(mon)) {
-        /*
-         * If it returns failure, it must have passed on error.
-         *
-         * Action: Report an internal error to the client if in QMP.
-         */
-        qerror_report(QERR_UNDEFINED_ERROR);
-    }
-}
-
 static void handle_user_command(Monitor *mon, const char *cmdline)
 {
     QDict *qdict;
@@ -5015,28 +5003,17 @@  static QDict *qmp_check_input_obj(QObject *input_obj)
     return input_dict;
 }
 
-static void qmp_call_cmd(Monitor *mon, const mon_cmd_t *cmd,
-                         const QDict *params)
-{
-    int ret;
-    QObject *data = NULL;
-
-    ret = cmd->mhandler.cmd_new(mon, params, &data);
-    handler_audit(mon, cmd, ret);
-    monitor_protocol_emitter(mon, data);
-    qobject_decref(data);
-}
-
 static void handle_qmp_command(JSONMessageParser *parser, QList *tokens)
 {
     int err;
-    QObject *obj;
+    QObject *obj, *data;
     QDict *input, *args;
     const mon_cmd_t *cmd;
     const char *cmd_name;
     Monitor *mon = cur_mon;
 
     args = input = NULL;
+    data = NULL;
 
     obj = json_parser_parse(tokens, NULL);
     if (!obj) {
@@ -5079,12 +5056,17 @@  static void handle_qmp_command(JSONMessageParser *parser, QList *tokens)
         goto err_out;
     }
 
-    qmp_call_cmd(mon, cmd, args);
-    goto out;
+    if (cmd->mhandler.cmd_new(mon, args, &data)) {
+        /* Command failed... */
+        if (!monitor_has_error(mon)) {
+            /* ... without setting an error, so make one up */
+            qerror_report(QERR_UNDEFINED_ERROR);
+        }
+    }
 
 err_out:
-    monitor_protocol_emitter(mon, NULL);
-out:
+    monitor_protocol_emitter(mon, data);
+    qobject_decref(data);
     QDECREF(input);
     QDECREF(args);
 }