Re: [Qemu-devel] [PATCH 11/11] migration: Add migration events on target side

["Dr. David Alan Gilbert" <dgilbert@redhat.com>]

* Eric Blake (eblake@redhat.com) wrote:
> On 06/18/2015 04:53 AM, Dr. David Alan Gilbert wrote:
> > * Juan Quintela (quintela@redhat.com) wrote:
> >> We reuse the migration events from the source side, sending them on the
> >> appropiate place.
> 
> s/appropiate/appropriate/
> 
> >>
> >> Signed-off-by: Juan Quintela <quintela@redhat.com>
> >> Reviewed-by: Eric Blake <eblake@redhat.com>
> >> ---
> >>  migration/migration.c | 5 ++++-
> >>  1 file changed, 4 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/migration/migration.c b/migration/migration.c
> >> index 3637d36..2b4fd55 100644
> >> --- a/migration/migration.c
> >> +++ b/migration/migration.c
> >> @@ -218,6 +218,7 @@ void qemu_start_incoming_migration(const char *uri, Error **errp)
> >>  {
> >>      const char *p;
> >>
> >> +    qapi_event_send_migration(MIGRATION_STATUS_SETUP, &error_abort);
> > 
> > Try and avoid error_abort - you don't want to trigger an assert (and associated
> > core etc) if it's just something like the monitor disconnecting.
> > (And anyway in this case you have an errp).
> 
> But this use is fine, matching the idiom of ALL OTHER qapi_event_send_*
> calls.  (Arguably, if sending an event can never fail, then maybe we
> shouldn't have made it a parameter; OOM failures already abort, and if
> the only other possible failure is malformed json but the whole point of
> a generated code guarantees that we cannot hit that bug, or if the only
> failure is a disconnected monitor but you can't report the error because
> you have no monitor left, then being able to catch an error doesn't help).

I think you're right the current stuff hung off qapi_event_send_migration never
uses the error pointer.  Still, it would be nice to try and avoid error_abort
where possible; you can see some implementation of an event sender deciding
to throw an error if it can't write to whatever event log it's got, and then
you don't want to cause an assert() - you might want an error printed and an 
exit, but you dont want an assert.

Anyway, since as you say, since all callers are equally broken, and this
was my only issue with this patch:

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

Dave

> 
> -- 
> Eric Blake   eblake redhat com    +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
> 


--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK