Patchwork [3/3] monitor: Convert do_migrate() to QError

login
register
mail settings
Submitter Markus Armbruster
Date March 23, 2010, 6:07 p.m.
Message ID <1269367641-6241-4-git-send-email-armbru@redhat.com>
Download mbox | patch
Permalink /patch/48358/
State New
Headers show

Comments

Markus Armbruster - March 23, 2010, 6:07 p.m.
Human monitor error message changes from "unknown migration protocol:
FOO" to "Invalid parameter uri".

The conversion is shallow: the FOO_start_outgoing_migration() aren't
converted.  Converting them is a big job for relatively little
practical benefit, so leave it for later.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 migration.c |    9 +++++----
 1 files changed, 5 insertions(+), 4 deletions(-)
Luiz Capitulino - March 24, 2010, 7:40 p.m.
On Tue, 23 Mar 2010 19:07:21 +0100
Markus Armbruster <armbru@redhat.com> wrote:

> Human monitor error message changes from "unknown migration protocol:
> FOO" to "Invalid parameter uri".
> 
> The conversion is shallow: the FOO_start_outgoing_migration() aren't
> converted.  Converting them is a big job for relatively little
> practical benefit, so leave it for later.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  migration.c |    9 +++++----
>  1 files changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/migration.c b/migration.c
> index 05f6cc5..47d2ab5 100644
> --- a/migration.c
> +++ b/migration.c
> @@ -56,14 +56,14 @@ void qemu_start_incoming_migration(const char *uri)
>  
>  int do_migrate(Monitor *mon, const QDict *qdict, QObject **ret_data)
>  {
> -    MigrationState *s = NULL;
> +    MigrationState *s;
>      const char *p;
>      int detach = qdict_get_int(qdict, "detach");
>      const char *uri = qdict_get_str(qdict, "uri");
>  
>      if (current_migration &&
>          current_migration->get_status(current_migration) == MIG_STATE_ACTIVE) {
> -        monitor_printf(mon, "migration already in progress\n");
> +        qerror_report(QERR_MIGRATION_IN_PROGRESS);
>          return -1;
>      }

 What about QERR_OPERATION_IN_PROGRESS? So that we have:

"Operation already in progress: migration".

>  
> @@ -86,12 +86,13 @@ int do_migrate(Monitor *mon, const QDict *qdict, QObject **ret_data)
>                                          (int)qdict_get_int(qdict, "inc"));
>  #endif
>      } else {
> -        monitor_printf(mon, "unknown migration protocol: %s\n", uri);
> +        qerror_report(QERR_INVALID_PARAMETER, "uri");
>          return -1;
>      }
>  
>      if (s == NULL) {
> -        monitor_printf(mon, "migration failed\n");
> +        /* TODO push error reporting into the FOO_start_outgoing_migration() */
> +        qerror_report(QERR_MIGRATION_FAILED);
>          return -1;
>      }

 I think this one is no better than the automatic UndefinedError
which is going to be triggered. I would only touch this when/if
we get the migration functions converted.
Markus Armbruster - March 25, 2010, 5:37 p.m.
Luiz Capitulino <lcapitulino@redhat.com> writes:

> On Tue, 23 Mar 2010 19:07:21 +0100
> Markus Armbruster <armbru@redhat.com> wrote:
>
>> Human monitor error message changes from "unknown migration protocol:
>> FOO" to "Invalid parameter uri".
>> 
>> The conversion is shallow: the FOO_start_outgoing_migration() aren't
>> converted.  Converting them is a big job for relatively little
>> practical benefit, so leave it for later.
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  migration.c |    9 +++++----
>>  1 files changed, 5 insertions(+), 4 deletions(-)
>> 
>> diff --git a/migration.c b/migration.c
>> index 05f6cc5..47d2ab5 100644
>> --- a/migration.c
>> +++ b/migration.c
>> @@ -56,14 +56,14 @@ void qemu_start_incoming_migration(const char *uri)
>>  
>>  int do_migrate(Monitor *mon, const QDict *qdict, QObject **ret_data)
>>  {
>> -    MigrationState *s = NULL;
>> +    MigrationState *s;
>>      const char *p;
>>      int detach = qdict_get_int(qdict, "detach");
>>      const char *uri = qdict_get_str(qdict, "uri");
>>  
>>      if (current_migration &&
>>          current_migration->get_status(current_migration) == MIG_STATE_ACTIVE) {
>> -        monitor_printf(mon, "migration already in progress\n");
>> +        qerror_report(QERR_MIGRATION_IN_PROGRESS);
>>          return -1;
>>      }
>
>  What about QERR_OPERATION_IN_PROGRESS? So that we have:
>
> "Operation already in progress: migration".

We'd get an error argument "activity": "migration".  Fine with me.

>> @@ -86,12 +86,13 @@ int do_migrate(Monitor *mon, const QDict *qdict, QObject **ret_data)
>>                                          (int)qdict_get_int(qdict, "inc"));
>>  #endif
>>      } else {
>> -        monitor_printf(mon, "unknown migration protocol: %s\n", uri);
>> +        qerror_report(QERR_INVALID_PARAMETER, "uri");
>>          return -1;
>>      }
>>  
>>      if (s == NULL) {
>> -        monitor_printf(mon, "migration failed\n");
>> +        /* TODO push error reporting into the FOO_start_outgoing_migration() */
>> +        qerror_report(QERR_MIGRATION_FAILED);
>>          return -1;
>>      }
>
>  I think this one is no better than the automatic UndefinedError
> which is going to be triggered. I would only touch this when/if
> we get the migration functions converted.

I feel it is a bit better, because:

* It doesn't dilute the nice "this is a bug, and I should report it"
  property of UndefinedError.

* It avoids the "returned failure but did not pass an error" message.
  Minor, because it's under CONFIG_DEBUG_MONITOR.

But I'm not particular about it.
Luiz Capitulino - March 25, 2010, 5:49 p.m.
On Thu, 25 Mar 2010 18:37:25 +0100
Markus Armbruster <armbru@redhat.com> wrote:

[...]

> >> @@ -86,12 +86,13 @@ int do_migrate(Monitor *mon, const QDict *qdict, QObject **ret_data)
> >>                                          (int)qdict_get_int(qdict, "inc"));
> >>  #endif
> >>      } else {
> >> -        monitor_printf(mon, "unknown migration protocol: %s\n", uri);
> >> +        qerror_report(QERR_INVALID_PARAMETER, "uri");
> >>          return -1;
> >>      }
> >>  
> >>      if (s == NULL) {
> >> -        monitor_printf(mon, "migration failed\n");
> >> +        /* TODO push error reporting into the FOO_start_outgoing_migration() */
> >> +        qerror_report(QERR_MIGRATION_FAILED);
> >>          return -1;
> >>      }
> >
> >  I think this one is no better than the automatic UndefinedError
> > which is going to be triggered. I would only touch this when/if
> > we get the migration functions converted.
> 
> I feel it is a bit better, because:
> 
> * It doesn't dilute the nice "this is a bug, and I should report it"
>   property of UndefinedError.
> 
> * It avoids the "returned failure but did not pass an error" message.
>   Minor, because it's under CONFIG_DEBUG_MONITOR.

 But this is exactly what we want because having QERR_MIGRATION_FAILED
there doesn't fix the problems those warnings are making us aware of.
Markus Armbruster - March 25, 2010, 7:30 p.m.
Luiz Capitulino <lcapitulino@redhat.com> writes:

> On Thu, 25 Mar 2010 18:37:25 +0100
> Markus Armbruster <armbru@redhat.com> wrote:
>
> [...]
>
>> >> @@ -86,12 +86,13 @@ int do_migrate(Monitor *mon, const QDict *qdict, QObject **ret_data)
>> >>                                          (int)qdict_get_int(qdict, "inc"));
>> >>  #endif
>> >>      } else {
>> >> -        monitor_printf(mon, "unknown migration protocol: %s\n", uri);
>> >> +        qerror_report(QERR_INVALID_PARAMETER, "uri");
>> >>          return -1;
>> >>      }
>> >>  
>> >>      if (s == NULL) {
>> >> -        monitor_printf(mon, "migration failed\n");
>> >> +        /* TODO push error reporting into the FOO_start_outgoing_migration() */
>> >> +        qerror_report(QERR_MIGRATION_FAILED);
>> >>          return -1;
>> >>      }
>> >
>> >  I think this one is no better than the automatic UndefinedError
>> > which is going to be triggered. I would only touch this when/if
>> > we get the migration functions converted.
>> 
>> I feel it is a bit better, because:
>> 
>> * It doesn't dilute the nice "this is a bug, and I should report it"
>>   property of UndefinedError.
>> 
>> * It avoids the "returned failure but did not pass an error" message.
>>   Minor, because it's under CONFIG_DEBUG_MONITOR.
>
>  But this is exactly what we want because having QERR_MIGRATION_FAILED
> there doesn't fix the problems those warnings are making us aware of.

Except we are already aware of the problem, and additional warnings are
just noise.
Luiz Capitulino - March 26, 2010, 12:31 p.m.
On Thu, 25 Mar 2010 20:30:33 +0100
Markus Armbruster <armbru@redhat.com> wrote:

> Luiz Capitulino <lcapitulino@redhat.com> writes:
> 
> > On Thu, 25 Mar 2010 18:37:25 +0100
> > Markus Armbruster <armbru@redhat.com> wrote:
> >
> > [...]
> >
> >> >> @@ -86,12 +86,13 @@ int do_migrate(Monitor *mon, const QDict *qdict, QObject **ret_data)
> >> >>                                          (int)qdict_get_int(qdict, "inc"));
> >> >>  #endif
> >> >>      } else {
> >> >> -        monitor_printf(mon, "unknown migration protocol: %s\n", uri);
> >> >> +        qerror_report(QERR_INVALID_PARAMETER, "uri");
> >> >>          return -1;
> >> >>      }
> >> >>  
> >> >>      if (s == NULL) {
> >> >> -        monitor_printf(mon, "migration failed\n");
> >> >> +        /* TODO push error reporting into the FOO_start_outgoing_migration() */
> >> >> +        qerror_report(QERR_MIGRATION_FAILED);
> >> >>          return -1;
> >> >>      }
> >> >
> >> >  I think this one is no better than the automatic UndefinedError
> >> > which is going to be triggered. I would only touch this when/if
> >> > we get the migration functions converted.
> >> 
> >> I feel it is a bit better, because:
> >> 
> >> * It doesn't dilute the nice "this is a bug, and I should report it"
> >>   property of UndefinedError.
> >> 
> >> * It avoids the "returned failure but did not pass an error" message.
> >>   Minor, because it's under CONFIG_DEBUG_MONITOR.
> >
> >  But this is exactly what we want because having QERR_MIGRATION_FAILED
> > there doesn't fix the problems those warnings are making us aware of.
> 
> Except we are already aware of the problem, and additional warnings are
> just noise.

 Our memory is not the best place for it, not only because we can forget,
but also because it's limited on you and me.

 Anyone who enables QMP's debugging support should be able to know what's
there to be done.
Markus Armbruster - March 29, 2010, 12:38 p.m.
Luiz Capitulino <lcapitulino@redhat.com> writes:

> On Thu, 25 Mar 2010 20:30:33 +0100
> Markus Armbruster <armbru@redhat.com> wrote:
>
>> Luiz Capitulino <lcapitulino@redhat.com> writes:
>> 
>> > On Thu, 25 Mar 2010 18:37:25 +0100
>> > Markus Armbruster <armbru@redhat.com> wrote:
>> >
>> > [...]
>> >
>> >> >> @@ -86,12 +86,13 @@ int do_migrate(Monitor *mon, const QDict *qdict, QObject **ret_data)
>> >> >>                                          (int)qdict_get_int(qdict, "inc"));
>> >> >>  #endif
>> >> >>      } else {
>> >> >> -        monitor_printf(mon, "unknown migration protocol: %s\n", uri);
>> >> >> +        qerror_report(QERR_INVALID_PARAMETER, "uri");
>> >> >>          return -1;
>> >> >>      }
>> >> >>  
>> >> >>      if (s == NULL) {
>> >> >> -        monitor_printf(mon, "migration failed\n");
>> >> >> +        /* TODO push error reporting into the FOO_start_outgoing_migration() */
>> >> >> +        qerror_report(QERR_MIGRATION_FAILED);
>> >> >>          return -1;
>> >> >>      }
>> >> >
>> >> >  I think this one is no better than the automatic UndefinedError
>> >> > which is going to be triggered. I would only touch this when/if
>> >> > we get the migration functions converted.
>> >> 
>> >> I feel it is a bit better, because:
>> >> 
>> >> * It doesn't dilute the nice "this is a bug, and I should report it"
>> >>   property of UndefinedError.
>> >> 
>> >> * It avoids the "returned failure but did not pass an error" message.
>> >>   Minor, because it's under CONFIG_DEBUG_MONITOR.
>> >
>> >  But this is exactly what we want because having QERR_MIGRATION_FAILED
>> > there doesn't fix the problems those warnings are making us aware of.
>> 
>> Except we are already aware of the problem, and additional warnings are
>> just noise.
>
>  Our memory is not the best place for it, not only because we can forget,
> but also because it's limited on you and me.
>
>  Anyone who enables QMP's debugging support should be able to know what's
> there to be done.

That's what TODO comments are for.
Luiz Capitulino - March 29, 2010, 1:31 p.m.
On Mon, 29 Mar 2010 14:38:35 +0200
Markus Armbruster <armbru@redhat.com> wrote:

> Luiz Capitulino <lcapitulino@redhat.com> writes:
> 
> > On Thu, 25 Mar 2010 20:30:33 +0100
> > Markus Armbruster <armbru@redhat.com> wrote:
> >
> >> Luiz Capitulino <lcapitulino@redhat.com> writes:
> >> 
> >> > On Thu, 25 Mar 2010 18:37:25 +0100
> >> > Markus Armbruster <armbru@redhat.com> wrote:
> >> >
> >> > [...]
> >> >
> >> >> >> @@ -86,12 +86,13 @@ int do_migrate(Monitor *mon, const QDict *qdict, QObject **ret_data)
> >> >> >>                                          (int)qdict_get_int(qdict, "inc"));
> >> >> >>  #endif
> >> >> >>      } else {
> >> >> >> -        monitor_printf(mon, "unknown migration protocol: %s\n", uri);
> >> >> >> +        qerror_report(QERR_INVALID_PARAMETER, "uri");
> >> >> >>          return -1;
> >> >> >>      }
> >> >> >>  
> >> >> >>      if (s == NULL) {
> >> >> >> -        monitor_printf(mon, "migration failed\n");
> >> >> >> +        /* TODO push error reporting into the FOO_start_outgoing_migration() */
> >> >> >> +        qerror_report(QERR_MIGRATION_FAILED);
> >> >> >>          return -1;
> >> >> >>      }
> >> >> >
> >> >> >  I think this one is no better than the automatic UndefinedError
> >> >> > which is going to be triggered. I would only touch this when/if
> >> >> > we get the migration functions converted.
> >> >> 
> >> >> I feel it is a bit better, because:
> >> >> 
> >> >> * It doesn't dilute the nice "this is a bug, and I should report it"
> >> >>   property of UndefinedError.
> >> >> 
> >> >> * It avoids the "returned failure but did not pass an error" message.
> >> >>   Minor, because it's under CONFIG_DEBUG_MONITOR.
> >> >
> >> >  But this is exactly what we want because having QERR_MIGRATION_FAILED
> >> > there doesn't fix the problems those warnings are making us aware of.
> >> 
> >> Except we are already aware of the problem, and additional warnings are
> >> just noise.
> >
> >  Our memory is not the best place for it, not only because we can forget,
> > but also because it's limited on you and me.
> >
> >  Anyone who enables QMP's debugging support should be able to know what's
> > there to be done.
> 
> That's what TODO comments are for.

 It's case by case and in this very case I'd only add QERR_MIGRATION_FAILED
if it makes difference for clients, because besides silencing the warning
w/o the proper fix, this error is probably going to be dropped when the
*_outgoing_migration() functions are converted.

Patch

diff --git a/migration.c b/migration.c
index 05f6cc5..47d2ab5 100644
--- a/migration.c
+++ b/migration.c
@@ -56,14 +56,14 @@  void qemu_start_incoming_migration(const char *uri)
 
 int do_migrate(Monitor *mon, const QDict *qdict, QObject **ret_data)
 {
-    MigrationState *s = NULL;
+    MigrationState *s;
     const char *p;
     int detach = qdict_get_int(qdict, "detach");
     const char *uri = qdict_get_str(qdict, "uri");
 
     if (current_migration &&
         current_migration->get_status(current_migration) == MIG_STATE_ACTIVE) {
-        monitor_printf(mon, "migration already in progress\n");
+        qerror_report(QERR_MIGRATION_IN_PROGRESS);
         return -1;
     }
 
@@ -86,12 +86,13 @@  int do_migrate(Monitor *mon, const QDict *qdict, QObject **ret_data)
                                         (int)qdict_get_int(qdict, "inc"));
 #endif
     } else {
-        monitor_printf(mon, "unknown migration protocol: %s\n", uri);
+        qerror_report(QERR_INVALID_PARAMETER, "uri");
         return -1;
     }
 
     if (s == NULL) {
-        monitor_printf(mon, "migration failed\n");
+        /* TODO push error reporting into the FOO_start_outgoing_migration() */
+        qerror_report(QERR_MIGRATION_FAILED);
         return -1;
     }