From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1O4VQD-0004pQ-0t for qemu-devel@nongnu.org; Wed, 21 Apr 2010 04:36:37 -0400 Received: from [140.186.70.92] (port=43612 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1O4VQB-0004mI-5p for qemu-devel@nongnu.org; Wed, 21 Apr 2010 04:36:36 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.69) (envelope-from ) id 1O4VQ9-0000dA-6c for qemu-devel@nongnu.org; Wed, 21 Apr 2010 04:36:35 -0400 Received: from mx1.redhat.com ([209.132.183.28]:40315) by eggs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1O4VQ8-0000cu-SR for qemu-devel@nongnu.org; Wed, 21 Apr 2010 04:36:33 -0400 Received: from int-mx04.intmail.prod.int.phx2.redhat.com (int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.17]) by mx1.redhat.com (8.13.8/8.13.8) with ESMTP id o3L8aVgu031117 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Wed, 21 Apr 2010 04:36:32 -0400 From: Juan Quintela In-Reply-To: <20100420185959.31829121@redhat.com> (Luiz Capitulino's message of "Tue, 20 Apr 2010 18:59:59 -0300") References: <1271797792-24571-1-git-send-email-lcapitulino@redhat.com> <1271797792-24571-5-git-send-email-lcapitulino@redhat.com> <20100420185959.31829121@redhat.com> Date: Wed, 21 Apr 2010 10:36:29 +0200 Message-ID: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Subject: [Qemu-devel] Re: [PATCH 04/22] savevm: do_loadvm(): Always resume the VM List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Luiz Capitulino Cc: kwolf@redhat.com, qemu-devel@nongnu.org, armbru@redhat.com Luiz Capitulino wrote: > On Tue, 20 Apr 2010 23:28:23 +0200 > Juan Quintela wrote: > >> Luiz Capitulino wrote: >> > do_loadvm(), which implements the 'loadvm' Monitor command, pauses >> > the emulation to load the saved VM, however it will only resume >> > it if the loading succeeds. >> > >> > In other words, if the user issues 'loadvm' and it fails, the >> > end result will be an error message and a paused VM. >> > >> > This seems an undesirable side effect to me because, most of the >> > time, if a Monitor command fails the best thing we can do is to >> > leave the VM as it were before the command was executed. >> > >> > FIXME: This will try to run a potentially corrupted image, the >> > solution is to split load_vmstate() in two and only keep >> > the VM paused if qemu_loadvm_state() fails. >> >> Any of the other errors in loadvm also requires you to not load the >> state. > > Really? Everything that happens before qemu_fopen_bdrv() seems to be > only looking for the snapshot.. Let's see: bs = get_bs_snapshots(); if (!bs) { error_report("No block device supports snapshots"); return -EINVAL; } //// If we are asked to load a vm and there are no snapshots on any disk //// ... trying to run the image look overkill /* Flush all IO requests so they don't interfere with the new state. */ qemu_aio_flush(); QTAILQ_FOREACH(dinfo, &drives, next) { bs1 = dinfo->bdrv; if (bdrv_has_snapshot(bs1)) { /// We found a device that has snapshots ret = bdrv_snapshot_goto(bs1, name); if (ret < 0) { /// And don't have a snapshot with the name that we wanted switch(ret) { case -ENOTSUP: error_report("%sSnapshots not supported on device '%s'", bs != bs1 ? "Warning: " : "", bdrv_get_device_name(bs1)); break; case -ENOENT: error_report("%sCould not find snapshot '%s' on device '%s'", bs != bs1 ? "Warning: " : "", name, bdrv_get_device_name(bs1)); break; default: error_report("%sError %d while activating snapshot on '%s'", bs != bs1 ? "Warning: " : "", ret, bdrv_get_device_name(bs1)); break; } /* fatal on snapshot block device */ // I think that one inconditional exit with predjuice could be in order here // Notice that bdrv_snapshot_goto() modifies the disk, name is as bad as // you can get. It just open the disk, opens the snapshot, increases // its counter of users, and makes it available for use after here // (i.e. loading state, posibly conflicting with previous running // VM a.k.a. disk corruption. if (bs == bs1) return 0; // This error is as bad as it can gets :( We have to load a vmstate, // and the disk that should have the memory image don't have it. // This is an error, I just put the wrong nunmber the previous time. // Notice that this error should be very rare. } } } As stated, I don't think that trying to run the machine at any point would make any sense. Only case where it is safe to run it is if the failure is at get_bs_snapshots(), but at that point running the machine means: $ loadvm other_image Error "other_image" snapshot don't exist. $ running the previous VM looks like something that should be done explicitely. If the error happened after that get_bs_snapshots(), We would need a new flag to just refuse to continue. Only valid operations at that point are other loadvm operations, i.e. our state is wrong one way or another. > My understanding is that the loading only happens in qemu_loadvm_state(), > is this wrong? As stated on description, don't make sense that split. It all case, what we need is the new flag to not allow other run operations other than loadvm. Later, Juan.