From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:36220) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z5ZIT-0007cS-UU for qemu-devel@nongnu.org; Thu, 18 Jun 2015 08:52:02 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Z5ZIP-0006c8-IT for qemu-devel@nongnu.org; Thu, 18 Jun 2015 08:51:57 -0400 Received: from mx1.redhat.com ([209.132.183.28]:44782) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z5ZIP-0006br-DT for qemu-devel@nongnu.org; Thu, 18 Jun 2015 08:51:53 -0400 Received: from int-mx14.intmail.prod.int.phx2.redhat.com (int-mx14.intmail.prod.int.phx2.redhat.com [10.5.11.27]) by mx1.redhat.com (Postfix) with ESMTPS id C1E21362082 for ; Thu, 18 Jun 2015 12:51:52 +0000 (UTC) Date: Thu, 18 Jun 2015 13:51:48 +0100 From: "Dr. David Alan Gilbert" Message-ID: <20150618125148.GJ2248@work-vm> References: <1434505833-11234-1-git-send-email-quintela@redhat.com> <1434505833-11234-12-git-send-email-quintela@redhat.com> <20150618105312.GH2248@work-vm> <5582B75A.3010402@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <5582B75A.3010402@redhat.com> Subject: Re: [Qemu-devel] [PATCH 11/11] migration: Add migration events on target side List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake Cc: amit.shah@redhat.com, qemu-devel@nongnu.org, Juan Quintela * 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 > >> Reviewed-by: Eric Blake > >> --- > >> 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 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