| Message ID | 1269367641-6241-4-git-send-email-armbru@redhat.com |
|---|---|
| State | New |
| Headers | show |
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.
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.
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.
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.
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.
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.
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.
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; }
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(-)