| Message ID | 1279132051-15865-1-git-send-email-miguel.filho@gmail.com |
|---|---|
| State | New |
| Headers | show |
Am 14.07.2010 20:27, schrieb Miguel Di Ciurcio Filho: > This patch improves the resilience of the load_vmstate() function, doing > further and better ordered tests. > > In load_vmstate(), if there is any error on bdrv_snapshot_goto(), except if the > error is on VM state device, load_vmstate() will return zero and the VM will be > started with major corruption chances. > > The current process: > - test if there is any writable device without snapshot support > - if exists return -error > - get the device that saves the VM state, possible return -error but unlikely > because it was tested earlier > - flush I/O > - run bdrv_snapshot_goto() on devices > - if fails, give an warning and goes to the next (not good!) > - if fails on the VM state device, return zero (not good!) > - check if the requested snapshot exists on the device that saves the VM state > and the state is not zero > - if fails return -error > - open the file with the VM state > - if fails return -error > - load the VM state > - if fails return -error > - return zero > > New behavior: > - get the device that saves the VM state > - if fails return -error > - check if the requested snapshot exists on the device that saves the VM state > and the state is not zero > - if fails return -error > - test if there is any writable device without snapshot support > - if exists return -error > - test if the devices with snapshot support have the requested snapshot > - if anyone fails, return -error > - flush I/O > - run snapshot_goto() on devices > - if anyone fails, return -error > - open the file with the VM state > - if fails return -error > - load the VM state > - if fails return -error > - return zero > > do_loadvm must not call vm_start if any error has occurred in load_vmstate. > > Signed-off-by: Miguel Di Ciurcio Filho <miguel.filho@gmail.com> > --- > monitor.c | 3 +- > savevm.c | 71 ++++++++++++++++++++++++++++-------------------------------- > 2 files changed, 35 insertions(+), 39 deletions(-) > > diff --git a/monitor.c b/monitor.c > index 45fd482..aa60cfa 100644 > --- a/monitor.c > +++ b/monitor.c > @@ -2270,8 +2270,9 @@ static void do_loadvm(Monitor *mon, const QDict *qdict) > > vm_stop(0); > > - if (load_vmstate(name) >= 0 && saved_vm_running) > + if (load_vmstate(name) == 0 && saved_vm_running) { > vm_start(); > + } > } > > int monitor_get_fd(Monitor *mon, const char *fdname) > diff --git a/savevm.c b/savevm.c > index ee27989..9f29cb0 100644 > --- a/savevm.c > +++ b/savevm.c > @@ -1804,12 +1804,25 @@ void do_savevm(Monitor *mon, const QDict *qdict) > > int load_vmstate(const char *name) > { > - BlockDriverState *bs, *bs1; > + BlockDriverState *bs, *bs_vm_state; > QEMUSnapshotInfo sn; > QEMUFile *f; > int ret; > > - /* Verify if there is a device that doesn't support snapshots and is writable */ > + bs_vm_state = bdrv_snapshots(); > + if (!bs_vm_state) { > + error_report("No block device supports snapshots"); > + return -EINVAL; -ENOTSUP? > + } > + > + /* Don't even try to load empty VM states */ > + ret = bdrv_snapshot_find(bs_vm_state, &sn, name); > + if ((ret >= 0) && (sn.vm_state_size == 0)) { > + return -EINVAL; > + } You can probably stop here already if ret < 0: ret = ... if (ret < 0) { return ret; } else if (sn.vm_state_size == 0) { return -EINVAL; } I'm not sure about EINVAL here either, but I don't really have a better suggestion. > + > + /* Verify if there is any device that doesn't support snapshots and is > + writable and check if the requested snapshot is available too. */ > bs = NULL; > while ((bs = bdrv_next(bs))) { > > @@ -1821,64 +1834,46 @@ int load_vmstate(const char *name) > error_report("Device '%s' is writable but does not support snapshots.", > bdrv_get_device_name(bs)); > return -ENOTSUP; > + } else { The then branch has a return, so you don't need the else here and can have the following code nested one level less. Looks good otherwise. Kevin
On Mon, Jul 19, 2010 at 11:22 AM, Kevin Wolf <kwolf@redhat.com> wrote: >> >> - /* Verify if there is a device that doesn't support snapshots and is writable */ >> + bs_vm_state = bdrv_snapshots(); >> + if (!bs_vm_state) { >> + error_report("No block device supports snapshots"); >> + return -EINVAL; > > -ENOTSUP? It was -EINVAL before, just kept it. But -ENOTSUP make more sense. >> + /* Don't even try to load empty VM states */ >> + ret = bdrv_snapshot_find(bs_vm_state, &sn, name); >> + if ((ret >= 0) && (sn.vm_state_size == 0)) { >> + return -EINVAL; >> + } > > You can probably stop here already if ret < 0: > > ret = ... > if (ret < 0) { > return ret; > } else if (sn.vm_state_size == 0) { > return -EINVAL; > } > Better indeed. >> >> @@ -1821,64 +1834,46 @@ int load_vmstate(const char *name) >> error_report("Device '%s' is writable but does not support snapshots.", >> bdrv_get_device_name(bs)); >> return -ENOTSUP; >> + } else { > > The then branch has a return, so you don't need the else here and can > have the following code nested one level less. > Ack. I will send v2 shortly. Thanks, Miguel
diff --git a/monitor.c b/monitor.c index 45fd482..aa60cfa 100644 --- a/monitor.c +++ b/monitor.c @@ -2270,8 +2270,9 @@ static void do_loadvm(Monitor *mon, const QDict *qdict) vm_stop(0); - if (load_vmstate(name) >= 0 && saved_vm_running) + if (load_vmstate(name) == 0 && saved_vm_running) { vm_start(); + } } int monitor_get_fd(Monitor *mon, const char *fdname) diff --git a/savevm.c b/savevm.c index ee27989..9f29cb0 100644 --- a/savevm.c +++ b/savevm.c @@ -1804,12 +1804,25 @@ void do_savevm(Monitor *mon, const QDict *qdict) int load_vmstate(const char *name) { - BlockDriverState *bs, *bs1; + BlockDriverState *bs, *bs_vm_state; QEMUSnapshotInfo sn; QEMUFile *f; int ret; - /* Verify if there is a device that doesn't support snapshots and is writable */ + bs_vm_state = bdrv_snapshots(); + if (!bs_vm_state) { + error_report("No block device supports snapshots"); + return -EINVAL; + } + + /* Don't even try to load empty VM states */ + ret = bdrv_snapshot_find(bs_vm_state, &sn, name); + if ((ret >= 0) && (sn.vm_state_size == 0)) { + return -EINVAL; + } + + /* Verify if there is any device that doesn't support snapshots and is + writable and check if the requested snapshot is available too. */ bs = NULL; while ((bs = bdrv_next(bs))) { @@ -1821,64 +1834,46 @@ int load_vmstate(const char *name) error_report("Device '%s' is writable but does not support snapshots.", bdrv_get_device_name(bs)); return -ENOTSUP; + } else { + ret = bdrv_snapshot_find(bs, &sn, name); + if (ret < 0) { + error_report("Device '%s' does not have the requested snapshot '%s'", + bdrv_get_device_name(bs), name); + return ret; + } } } - bs = bdrv_snapshots(); - if (!bs) { - error_report("No block device supports snapshots"); - return -EINVAL; - } - /* Flush all IO requests so they don't interfere with the new state. */ qemu_aio_flush(); - bs1 = NULL; - while ((bs1 = bdrv_next(bs1))) { - if (bdrv_can_snapshot(bs1)) { - ret = bdrv_snapshot_goto(bs1, name); + bs = NULL; + while ((bs = bdrv_next(bs))) { + if (bdrv_can_snapshot(bs)) { + ret = bdrv_snapshot_goto(bs, name); if (ret < 0) { - 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 */ - if (bs == bs1) - return 0; + error_report("Error %d while activating snapshot '%s' on '%s'", + ret, name, bdrv_get_device_name(bs)); + return ret; } } } - /* Don't even try to load empty VM states */ - ret = bdrv_snapshot_find(bs, &sn, name); - if ((ret >= 0) && (sn.vm_state_size == 0)) - return -EINVAL; - /* restore the VM state */ - f = qemu_fopen_bdrv(bs, 0); + f = qemu_fopen_bdrv(bs_vm_state, 0); if (!f) { error_report("Could not open VM state file"); return -EINVAL; } + ret = qemu_loadvm_state(f); + qemu_fclose(f); if (ret < 0) { error_report("Error %d while loading VM state", ret); return ret; } + return 0; }
This patch improves the resilience of the load_vmstate() function, doing further and better ordered tests. In load_vmstate(), if there is any error on bdrv_snapshot_goto(), except if the error is on VM state device, load_vmstate() will return zero and the VM will be started with major corruption chances. The current process: - test if there is any writable device without snapshot support - if exists return -error - get the device that saves the VM state, possible return -error but unlikely because it was tested earlier - flush I/O - run bdrv_snapshot_goto() on devices - if fails, give an warning and goes to the next (not good!) - if fails on the VM state device, return zero (not good!) - check if the requested snapshot exists on the device that saves the VM state and the state is not zero - if fails return -error - open the file with the VM state - if fails return -error - load the VM state - if fails return -error - return zero New behavior: - get the device that saves the VM state - if fails return -error - check if the requested snapshot exists on the device that saves the VM state and the state is not zero - if fails return -error - test if there is any writable device without snapshot support - if exists return -error - test if the devices with snapshot support have the requested snapshot - if anyone fails, return -error - flush I/O - run snapshot_goto() on devices - if anyone fails, return -error - open the file with the VM state - if fails return -error - load the VM state - if fails return -error - return zero do_loadvm must not call vm_start if any error has occurred in load_vmstate. Signed-off-by: Miguel Di Ciurcio Filho <miguel.filho@gmail.com> --- monitor.c | 3 +- savevm.c | 71 ++++++++++++++++++++++++++++-------------------------------- 2 files changed, 35 insertions(+), 39 deletions(-)