diff mbox

[v3,11/21] monitor: Propagate errors through invalid_qmp_mode()

Message ID 1432893420-18687-12-git-send-email-armbru@redhat.com
State New
Headers show

Commit Message

Markus Armbruster May 29, 2015, 9:56 a.m. UTC 
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 monitor.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

Comments

Luiz Capitulino May 29, 2015, 1:58 p.m. UTC | #1 
On Fri, 29 May 2015 11:56:50 +0200
Markus Armbruster <armbru@redhat.com> wrote:

> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  monitor.c | 18 ++++++++++--------
>  1 file changed, 10 insertions(+), 8 deletions(-)
> 
> diff --git a/monitor.c b/monitor.c
> index 61ea346..d336b8f 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -4708,19 +4708,20 @@ static int monitor_can_read(void *opaque)
>      return (mon->suspend_cnt == 0) ? 1 : 0;
>  }
>  
> -static bool invalid_qmp_mode(const Monitor *mon, const mon_cmd_t *cmd)
> +static bool invalid_qmp_mode(const Monitor *mon, const mon_cmd_t *cmd,
> +                             Error **errp)
>  {
>      bool is_cap = cmd->mhandler.cmd_new == do_qmp_capabilities;
>      if (is_cap && qmp_cmd_mode(mon)) {
> -        qerror_report(ERROR_CLASS_COMMAND_NOT_FOUND,
> -                      "Capabilities negotiation is already complete, command "
> -                      "'%s' ignored", cmd->name);
> +        error_set(errp, ERROR_CLASS_COMMAND_NOT_FOUND,
> +                  "Capabilities negotiation is already complete, command "
> +                  "'%s' ignored", cmd->name);
>          return true;
>      }
>      if (!is_cap && !qmp_cmd_mode(mon)) {
> -        qerror_report(ERROR_CLASS_COMMAND_NOT_FOUND,
> -                      "Expecting capabilities negotiation with "
> -                      "'qmp_capabilities' before command '%s'", cmd->name);
> +        error_set(errp, ERROR_CLASS_COMMAND_NOT_FOUND,
> +                  "Expecting capabilities negotiation with "
> +                  "'qmp_capabilities' before command '%s'", cmd->name);
>          return true;
>      }
>      return false;
> @@ -5010,7 +5011,8 @@ static void handle_qmp_command(JSONMessageParser *parser, QList *tokens)
>                        "The command %s has not been found", cmd_name);
>          goto err_out;
>      }
> -    if (invalid_qmp_mode(mon, cmd)) {
> +    if (invalid_qmp_mode(mon, cmd, &local_err)) {
> +        qerror_report_err(local_err);

I don't think you need to call qerror_report_err() here, and you could
have changed invalid_qmp_mode() to return void. You can make those changes
before applying if you choose to do them:

Reviewed-by: Luiz Capitulino <lcapitulino@redhat.com>

>          goto err_out;
>      }
>
Eric Blake May 29, 2015, 2:05 p.m. UTC | #2 
On 05/29/2015 03:56 AM, Markus Armbruster wrote:
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  monitor.c | 18 ++++++++++--------
>  1 file changed, 10 insertions(+), 8 deletions(-)

Reviewed-by: Eric Blake <eblake@redhat.com>
Markus Armbruster May 29, 2015, 2:19 p.m. UTC | #3 
Luiz Capitulino <lcapitulino@redhat.com> writes:

> On Fri, 29 May 2015 11:56:50 +0200
> Markus Armbruster <armbru@redhat.com> wrote:
>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  monitor.c | 18 ++++++++++--------
>>  1 file changed, 10 insertions(+), 8 deletions(-)
>> 
>> diff --git a/monitor.c b/monitor.c
>> index 61ea346..d336b8f 100644
>> --- a/monitor.c
>> +++ b/monitor.c
>> @@ -4708,19 +4708,20 @@ static int monitor_can_read(void *opaque)
>>      return (mon->suspend_cnt == 0) ? 1 : 0;
>>  }
>>  
>> -static bool invalid_qmp_mode(const Monitor *mon, const mon_cmd_t *cmd)
>> +static bool invalid_qmp_mode(const Monitor *mon, const mon_cmd_t *cmd,
>> +                             Error **errp)
>>  {
>>      bool is_cap = cmd->mhandler.cmd_new == do_qmp_capabilities;
>>      if (is_cap && qmp_cmd_mode(mon)) {
>> -        qerror_report(ERROR_CLASS_COMMAND_NOT_FOUND,
>> -                      "Capabilities negotiation is already complete, command "
>> -                      "'%s' ignored", cmd->name);
>> +        error_set(errp, ERROR_CLASS_COMMAND_NOT_FOUND,
>> +                  "Capabilities negotiation is already complete, command "
>> +                  "'%s' ignored", cmd->name);
>>          return true;
>>      }
>>      if (!is_cap && !qmp_cmd_mode(mon)) {
>> -        qerror_report(ERROR_CLASS_COMMAND_NOT_FOUND,
>> -                      "Expecting capabilities negotiation with "
>> -                      "'qmp_capabilities' before command '%s'", cmd->name);
>> +        error_set(errp, ERROR_CLASS_COMMAND_NOT_FOUND,
>> +                  "Expecting capabilities negotiation with "
>> +                  "'qmp_capabilities' before command '%s'", cmd->name);
>>          return true;
>>      }
>>      return false;
>> @@ -5010,7 +5011,8 @@ static void handle_qmp_command(JSONMessageParser *parser, QList *tokens)
>>                        "The command %s has not been found", cmd_name);
>>          goto err_out;
>>      }
>> -    if (invalid_qmp_mode(mon, cmd)) {
>> +    if (invalid_qmp_mode(mon, cmd, &local_err)) {
>> +        qerror_report_err(local_err);
>
> I don't think you need to call qerror_report_err() here, and you could
> have changed invalid_qmp_mode() to return void. You can make those changes
> before applying if you choose to do them:

Actually, I have to call it here, because after this patch, we still
have monitor_protocol_emitter() getting the error from mon->error, so we
still have to put it there.

However, I forgot to drop it along with the other calls in PATCH 14!

> Reviewed-by: Luiz Capitulino <lcapitulino@redhat.com>

Thanks!

>>          goto err_out;
>>      }
>>
Luiz Capitulino May 29, 2015, 2:27 p.m. UTC | #4 
On Fri, 29 May 2015 16:19:28 +0200
Markus Armbruster <armbru@redhat.com> wrote:

> Luiz Capitulino <lcapitulino@redhat.com> writes:
> 
> > On Fri, 29 May 2015 11:56:50 +0200
> > Markus Armbruster <armbru@redhat.com> wrote:
> >
> >> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> >> ---
> >>  monitor.c | 18 ++++++++++--------
> >>  1 file changed, 10 insertions(+), 8 deletions(-)
> >> 
> >> diff --git a/monitor.c b/monitor.c
> >> index 61ea346..d336b8f 100644
> >> --- a/monitor.c
> >> +++ b/monitor.c
> >> @@ -4708,19 +4708,20 @@ static int monitor_can_read(void *opaque)
> >>      return (mon->suspend_cnt == 0) ? 1 : 0;
> >>  }
> >>  
> >> -static bool invalid_qmp_mode(const Monitor *mon, const mon_cmd_t *cmd)
> >> +static bool invalid_qmp_mode(const Monitor *mon, const mon_cmd_t *cmd,
> >> +                             Error **errp)
> >>  {
> >>      bool is_cap = cmd->mhandler.cmd_new == do_qmp_capabilities;
> >>      if (is_cap && qmp_cmd_mode(mon)) {
> >> -        qerror_report(ERROR_CLASS_COMMAND_NOT_FOUND,
> >> -                      "Capabilities negotiation is already complete, command "
> >> -                      "'%s' ignored", cmd->name);
> >> +        error_set(errp, ERROR_CLASS_COMMAND_NOT_FOUND,
> >> +                  "Capabilities negotiation is already complete, command "
> >> +                  "'%s' ignored", cmd->name);
> >>          return true;
> >>      }
> >>      if (!is_cap && !qmp_cmd_mode(mon)) {
> >> -        qerror_report(ERROR_CLASS_COMMAND_NOT_FOUND,
> >> -                      "Expecting capabilities negotiation with "
> >> -                      "'qmp_capabilities' before command '%s'", cmd->name);
> >> +        error_set(errp, ERROR_CLASS_COMMAND_NOT_FOUND,
> >> +                  "Expecting capabilities negotiation with "
> >> +                  "'qmp_capabilities' before command '%s'", cmd->name);
> >>          return true;
> >>      }
> >>      return false;
> >> @@ -5010,7 +5011,8 @@ static void handle_qmp_command(JSONMessageParser *parser, QList *tokens)
> >>                        "The command %s has not been found", cmd_name);
> >>          goto err_out;
> >>      }
> >> -    if (invalid_qmp_mode(mon, cmd)) {
> >> +    if (invalid_qmp_mode(mon, cmd, &local_err)) {
> >> +        qerror_report_err(local_err);
> >
> > I don't think you need to call qerror_report_err() here, and you could
> > have changed invalid_qmp_mode() to return void. You can make those changes
> > before applying if you choose to do them:
> 
> Actually, I have to call it here, because after this patch, we still
> have monitor_protocol_emitter() getting the error from mon->error, so we
> still have to put it there.

You're right!

> However, I forgot to drop it along with the other calls in PATCH 14!

You don't have to respin because of that, feel free to fix it when
applying.

> 
> > Reviewed-by: Luiz Capitulino <lcapitulino@redhat.com>
> 
> Thanks!
> 
> >>          goto err_out;
> >>      }
> >>  
>
diff mbox

Patch

diff --git a/monitor.c b/monitor.c
index 61ea346..d336b8f 100644
--- a/monitor.c
+++ b/monitor.c
@@ -4708,19 +4708,20 @@  static int monitor_can_read(void *opaque)
     return (mon->suspend_cnt == 0) ? 1 : 0;
 }
 
-static bool invalid_qmp_mode(const Monitor *mon, const mon_cmd_t *cmd)
+static bool invalid_qmp_mode(const Monitor *mon, const mon_cmd_t *cmd,
+                             Error **errp)
 {
     bool is_cap = cmd->mhandler.cmd_new == do_qmp_capabilities;
     if (is_cap && qmp_cmd_mode(mon)) {
-        qerror_report(ERROR_CLASS_COMMAND_NOT_FOUND,
-                      "Capabilities negotiation is already complete, command "
-                      "'%s' ignored", cmd->name);
+        error_set(errp, ERROR_CLASS_COMMAND_NOT_FOUND,
+                  "Capabilities negotiation is already complete, command "
+                  "'%s' ignored", cmd->name);
         return true;
     }
     if (!is_cap && !qmp_cmd_mode(mon)) {
-        qerror_report(ERROR_CLASS_COMMAND_NOT_FOUND,
-                      "Expecting capabilities negotiation with "
-                      "'qmp_capabilities' before command '%s'", cmd->name);
+        error_set(errp, ERROR_CLASS_COMMAND_NOT_FOUND,
+                  "Expecting capabilities negotiation with "
+                  "'qmp_capabilities' before command '%s'", cmd->name);
         return true;
     }
     return false;
@@ -5010,7 +5011,8 @@  static void handle_qmp_command(JSONMessageParser *parser, QList *tokens)
                       "The command %s has not been found", cmd_name);
         goto err_out;
     }
-    if (invalid_qmp_mode(mon, cmd)) {
+    if (invalid_qmp_mode(mon, cmd, &local_err)) {
+        qerror_report_err(local_err);
         goto err_out;
     }