From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:54789) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RFoze-0003Dp-H2 for qemu-devel@nongnu.org; Mon, 17 Oct 2011 11:20:51 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1RFozY-00048O-EM for qemu-devel@nongnu.org; Mon, 17 Oct 2011 11:20:46 -0400 Received: from mail-yw0-f45.google.com ([209.85.213.45]:62695) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RFozY-00048F-7F for qemu-devel@nongnu.org; Mon, 17 Oct 2011 11:20:40 -0400 Received: by ywp17 with SMTP id 17so1318252ywp.4 for ; Mon, 17 Oct 2011 08:20:39 -0700 (PDT) Message-ID: <4E9C47C5.7030006@codemonkey.ws> Date: Mon, 17 Oct 2011 10:20:37 -0500 From: Anthony Liguori MIME-Version: 1.0 References: <4E9C367D.7020807@codemonkey.ws> In-Reply-To: Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 25/36] migration: Our release callback was just free List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: quintela@redhat.com Cc: qemu-devel@nongnu.org On 10/17/2011 10:12 AM, Juan Quintela wrote: > Anthony Liguori wrote: >> On 10/11/2011 05:00 AM, Juan Quintela wrote: >>> We called it from a single place, and always with state != >>> MIG_STATE_ACTIVE. Just remove the whole callback. For users of the >>> notifier, notice that this is exactly the case where they don't care, >>> we are just freeing the state from previous failed migration (it can't >>> be a sucessful one, otherwise we would not be running on that machine >>> in the first place). >>> >>> Signed-off-by: Juan Quintela >>> --- >>> migration.c | 19 +------------------ >>> migration.h | 1 - >>> 2 files changed, 1 insertions(+), 19 deletions(-) >>> >>> diff --git a/migration.c b/migration.c >>> index a8e936e..689464d 100644 >>> --- a/migration.c >>> +++ b/migration.c >>> @@ -123,10 +123,7 @@ int do_migrate(Monitor *mon, const QDict *qdict, QObject **ret_data) >>> goto free_migrate_state; >>> } >>> >>> - if (current_migration) { >>> - current_migration->release(current_migration); >>> - } >>> - >>> + g_free(current_migration); >> >> migrate_fd_cleanup() is no longer being called. > > It was never called. > >> Regards, >> >> Anthony Liguori >> >>> current_migration = s; >>> notifier_list_notify(&migration_state_notifiers, NULL); >>> return 0; >>> @@ -416,19 +413,6 @@ static void migrate_fd_cancel(MigrationState *s) >>> migrate_fd_cleanup(s); >>> } >>> >>> -static void migrate_fd_release(MigrationState *s) >>> -{ >>> - >>> - DPRINTF("releasing state\n"); >>> - >>> - if (s->state == MIG_STATE_ACTIVE) { > > The first thing that we look is that MIG_STATE_ACTIVE, and we call > migrate_fd_cleanup() on that case. It was meant to cancel existing migration but there was a later patch that didn't allow the implicit cancel anymore. So ignore my comments here. Regards, Anthony Liguori > > > See the beggining of do_migrate(). If we are in MIG_STATE_ACTIVE, we > just don't continue. the function. Notice that you complained about > comments being bad on the other patches, and in this very case, it was > explicitely stated on the comment O:-) > > int do_migrate(Monitor *mon, const QDict *qdict, QObject **ret_data) > { > MigrationState *s = NULL; > const char *p; > int detach = qdict_get_try_bool(qdict, "detach", 0); > int blk = qdict_get_try_bool(qdict, "blk", 0); > int inc = qdict_get_try_bool(qdict, "inc", 0); > 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"); > return -1; > > ************ We stop here **** > call to ->release() is below this. > } > > if (qemu_savevm_state_blocked(mon)) { > return -1; > } > > > >>> - s->state = MIG_STATE_CANCELLED; >>> - notifier_list_notify(&migration_state_notifiers, NULL); >>> - migrate_fd_cleanup(s); >>> - } >>> - g_free(s); >>> -} >>> - >>> static void migrate_fd_wait_for_unfreeze(void *opaque) >>> { >>> MigrationState *s = opaque; >>> @@ -511,7 +495,6 @@ static MigrationState *migrate_new(Monitor *mon, int64_t bandwidth_limit, >>> >>> s->cancel = migrate_fd_cancel; >>> s->get_status = migrate_fd_get_status; >>> - s->release = migrate_fd_release; >>> s->blk = blk; >>> s->shared = inc; >>> s->mon = NULL; >>> diff --git a/migration.h b/migration.h >>> index 3165140..1cdb539 100644 >>> --- a/migration.h >>> +++ b/migration.h >>> @@ -40,7 +40,6 @@ struct MigrationState >>> int (*write)(MigrationState *s, const void *buff, size_t size); >>> void (*cancel)(MigrationState *s); >>> int (*get_status)(MigrationState *s); >>> - void (*release)(MigrationState *s); >>> void *opaque; >>> int blk; >>> int shared; >