From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:52812) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z51sf-0005pC-OC for qemu-devel@nongnu.org; Tue, 16 Jun 2015 21:11:07 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Z51sa-0003u6-87 for qemu-devel@nongnu.org; Tue, 16 Jun 2015 21:11:05 -0400 Received: from mx1.redhat.com ([209.132.183.28]:44718) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z51sZ-0003u2-Pf for qemu-devel@nongnu.org; Tue, 16 Jun 2015 21:11:00 -0400 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by mx1.redhat.com (Postfix) with ESMTPS id 4034D2DC3F7 for ; Wed, 17 Jun 2015 01:10:59 +0000 (UTC) From: Juan Quintela In-Reply-To: <20150518102329.GD2201@work-vm> (David Alan Gilbert's message of "Mon, 18 May 2015 11:23:30 +0100") References: <1431620920-19710-1-git-send-email-quintela@redhat.com> <1431620920-19710-7-git-send-email-quintela@redhat.com> <20150518102329.GD2201@work-vm> Date: Wed, 17 Jun 2015 03:10:57 +0200 Message-ID: <87eglbgmvy.fsf@neno.neno> MIME-Version: 1.0 Content-Type: text/plain Subject: Re: [Qemu-devel] [PATCH 6/9] migration: create new section to store global state Reply-To: quintela@redhat.com List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Dr. David Alan Gilbert" Cc: qemu-devel@nongnu.org "Dr. David Alan Gilbert" wrote: > * Juan Quintela (quintela@redhat.com) wrote: >> This includes a new section that for now just stores the current qemu state. >> >> Right now, there are only one way to control what is the state of the >> target after migration. >> >> - If you run the target qemu with -S, it would start stopped. >> - If you run the target qemu without -S, it would run just after migration finishes. >> >> The problem here is what happens if we start the target without -S and >> there happens one error during migration that puts current state as >> -EIO. Migration would ends (notice that the error happend doing block >> IO, network IO, i.e. nothing related with migration), and when >> migration finish, we would just "continue" running on destination, >> probably hanging the guest/corruption data, whatever. >> >> Signed-off-by: Juan Quintela >> + >> +typedef struct { >> + int32_t size; >> + uint8_t runstate[100]; >> +} GlobalState; > > unsigned for size? Could use something smaller than a int32_t > I guess for a 100 byte buffer. uint8_t > >> + >> +static GlobalState global_state; >> + >> +void global_state_store(void) >> +{ >> + if (runstate_store((char *)global_state.runstate, >> + sizeof(global_state.runstate)) == -1) { >> + error_report("Runstate is too big: %s\n", global_state.runstate); > > error_report's don't need the \n done. > >> + exit(-1); >> + } >> +} >> + >> +char *global_state_get_runstate(void) >> +{ >> + return (char *)global_state.runstate; >> +} > > Is that needed outside of this file - if not then just make > it static or use the data directly? > State was born on vl.c, then I noticed that I could put it here, and forgot to remove globals. Don. >> + >> +static int global_state_post_load(void *opaque, int version_id) >> +{ >> + GlobalState *s = opaque; >> + int ret = 0; >> + char *runstate = (char *)s->runstate; >> + >> + if (strcmp(runstate, "running") != 0) { >> + >> + RunState r = runstate_index(runstate); >> + >> + if (r == -1) { >> + error_report("Unknown received state %s\n", runstate); > > error_report's don't need the \n > >> + return -1; >> + } >> + ret = vm_stop_force_state(r); > > Why do this here, rather than in process_incoming_migration_co; > that would seem neater to keep the state change in one place. If state is unknown for destination here, migration can still be aborted at this point, later that is not possible. Not possible in the sense that source would not noticed that destination has exited because it didn't understood something. But we could move that to incoming, just not clear that it is going to be easier, because then we have to do error control at a point that don't have it right now. Later, Juan. > > Dave > >> + } >> + >> + return ret; >> +} >> + >> +static void global_state_pre_save(void *opaque) >> +{ >> + GlobalState *s = opaque; >> + >> + s->size = strlen((char *)s->runstate) + 1; >> +} >> + >> +static const VMStateDescription vmstate_globalstate = { >> + .name = "globalstate", >> + .version_id = 1, >> + .minimum_version_id = 1, >> + .post_load = global_state_post_load, >> + .pre_save = global_state_pre_save, >> + .fields = (VMStateField[]) { >> + VMSTATE_INT32(size, GlobalState), >> + VMSTATE_BUFFER(runstate, GlobalState), >> + VMSTATE_END_OF_LIST() >> + }, >> +}; >> + >> +void register_global_state(void) >> +{ >> + /* We would use it independently that we receive it */ >> + strcpy((char *)&global_state.runstate, ""); >> + vmstate_register(NULL, 0, &vmstate_globalstate, &global_state); >> +} >> diff --git a/vl.c b/vl.c >> index b8844f6..8af6c79 100644 >> --- a/vl.c >> +++ b/vl.c >> @@ -4387,6 +4387,7 @@ int main(int argc, char **argv, char **envp) >> return 0; >> } >> >> + register_global_state(); >> if (incoming) { >> Error *local_err = NULL; >> qemu_start_incoming_migration(incoming, &local_err); >> -- >> 2.4.0 >> >> > -- > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK