[Qemu-devel] Re: [PATCH 3/3] monitor: Convert do_migrate() to QError
On Mon, 29 Mar 2010 14:38:35 +0200
Markus Armbruster wrote:
> Luiz Capitulino writes:
>
> > On Thu, 25 Mar 2010 20:30:33 +0100
> > Markus Armbruster wrote:
> >
> >> Luiz Capitulino writes:
> >>
> >> > On Thu, 25 Mar 2010 18:37:25 +0100
> >> > Markus Armbruster 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.
[Qemu-devel] Re: [PATCH 3/3] monitor: Convert do_migrate() to QError
Luiz Capitulino writes:
> On Thu, 25 Mar 2010 20:30:33 +0100
> Markus Armbruster wrote:
>
>> Luiz Capitulino writes:
>>
>> > On Thu, 25 Mar 2010 18:37:25 +0100
>> > Markus Armbruster 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
On Thu, 25 Mar 2010 20:30:33 +0100
Markus Armbruster wrote:
> Luiz Capitulino writes:
>
> > On Thu, 25 Mar 2010 18:37:25 +0100
> > Markus Armbruster 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
Luiz Capitulino writes:
> On Thu, 25 Mar 2010 18:37:25 +0100
> Markus Armbruster 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
On Thu, 25 Mar 2010 18:37:25 +0100
Markus Armbruster 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
Luiz Capitulino writes:
> On Tue, 23 Mar 2010 19:07:21 +0100
> Markus Armbruster 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
>> ---
>> 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
On Tue, 23 Mar 2010 19:07:21 +0100
Markus Armbruster 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
> ---
> 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.
