[Qemu-devel] [PATCH 0/3] Convert do_migrate() to QError

[Qemu-devel] [PATCH 0/3] Convert do_migrate() to QError

[Markus Armbruster]

Markus Armbruster (3):
  QError: New QERR_MIGRATION_IN_PROGRESS
  QError: New QERR_MIGRATION_FAILED
  monitor: Convert do_migrate() to QError

 migration.c |    9 +++++----
 qerror.c    |    8 ++++++++
 qerror.h    |    6 ++++++
 3 files changed, 19 insertions(+), 4 deletions(-)

[Qemu-devel] [PATCH 1/3] QError: New QERR_MIGRATION_IN_PROGRESS

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 qerror.c |    4 ++++
 qerror.h |    3 +++
 2 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/qerror.c b/qerror.c
index 8d885cd..560e70d 100644
--- a/qerror.c
+++ b/qerror.c
@@ -141,6 +141,10 @@ static const QErrorStringTable qerror_table[] = {
         .desc      = "Using KVM without %(capability), %(feature) unavailable",
     },
     {
+        .error_fmt = QERR_MIGRATION_IN_PROGRESS,
+        .desc      = "Migration already in progress"
+    },
+    {
         .error_fmt = QERR_MISSING_PARAMETER,
         .desc      = "Parameter '%(name)' is missing",
     },
diff --git a/qerror.h b/qerror.h
index bae08c0..ecc13e4 100644
--- a/qerror.h
+++ b/qerror.h
@@ -121,6 +121,9 @@ QError *qobject_to_qerror(const QObject *obj);
 #define QERR_KVM_MISSING_CAP \
     "{ 'class': 'KVMMissingCap', 'data': { 'capability': %s, 'feature': %s } }"
 
+#define QERR_MIGRATION_IN_PROGRESS \
+    "{ 'class': 'MigrationInProgress', 'data': {} }"
+
 #define QERR_MISSING_PARAMETER \
     "{ 'class': 'MissingParameter', 'data': { 'name': %s } }"
 
-- 
1.6.6.1

[Qemu-devel] [PATCH 2/3] QError: New QERR_MIGRATION_FAILED

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 qerror.c |    4 ++++
 qerror.h |    3 +++
 2 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/qerror.c b/qerror.c
index 560e70d..05ea9de 100644
--- a/qerror.c
+++ b/qerror.c
@@ -141,6 +141,10 @@ static const QErrorStringTable qerror_table[] = {
         .desc      = "Using KVM without %(capability), %(feature) unavailable",
     },
     {
+        .error_fmt = QERR_MIGRATION_FAILED,
+        .desc      = "Migration failed"
+    },
+    {
         .error_fmt = QERR_MIGRATION_IN_PROGRESS,
         .desc      = "Migration already in progress"
     },
diff --git a/qerror.h b/qerror.h
index ecc13e4..b0f909c 100644
--- a/qerror.h
+++ b/qerror.h
@@ -121,6 +121,9 @@ QError *qobject_to_qerror(const QObject *obj);
 #define QERR_KVM_MISSING_CAP \
     "{ 'class': 'KVMMissingCap', 'data': { 'capability': %s, 'feature': %s } }"
 
+#define QERR_MIGRATION_FAILED \
+    "{ 'class': 'MigrationFailed', 'data': {} }"
+
 #define QERR_MIGRATION_IN_PROGRESS \
     "{ 'class': 'MigrationInProgress', 'data': {} }"
 
-- 
1.6.6.1

[Qemu-devel] [PATCH 3/3] monitor: Convert do_migrate() to QError

[Markus Armbruster]

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;
     }
 
@@ -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;
     }
 
-- 
1.6.6.1

[Qemu-devel] Re: [PATCH 3/3] monitor: Convert do_migrate() to QError

[Luiz Capitulino]

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.

[Qemu-devel] Re: [PATCH 3/3] monitor: Convert do_migrate() to QError

[Markus Armbruster]

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.

[Qemu-devel] Re: [PATCH 3/3] monitor: Convert do_migrate() to QError

[Luiz Capitulino]

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.

[Qemu-devel] Re: [PATCH 3/3] monitor: Convert do_migrate() to QError

[Markus Armbruster]

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.

[Qemu-devel] Re: [PATCH 3/3] monitor: Convert do_migrate() to QError

[Luiz Capitulino]

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.

[Qemu-devel] Re: [PATCH 3/3] monitor: Convert do_migrate() to QError

[Markus Armbruster]

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.

[Qemu-devel] Re: [PATCH 3/3] monitor: Convert do_migrate() to QError

[Luiz Capitulino]

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.