[Qemu-devel] [PATCH 0/3] savem: various cleanups

[Qemu-devel] [PATCH 0/3] savem: various cleanups

[Miguel Di Ciurcio Filho]

This series does some cleanups and changes how the monitor command 'savevm'
handles overwriting snapshots with existing names and makes 'savevm' always
create a name for an snapshot being taken.

I'm keeping track of some issues with savevm/loadvm on a wiki page [1].

All feedback is very appreciated.

Regards,

Miguel

[1] http://wiki.qemu.org/Features/SnapshottingImprovements

Miguel Di Ciurcio Filho (3):
  cleanup: bdrv_snaphost_find() returns zero or -ENOENT
  cleanup: del_existing_snapshots() must return the upstream error code
  savevm: prevent snapshot overwriting and generate a default name

 qemu-monitor.hx |    9 ++++---
 savevm.c        |   68 ++++++++++++++++++++++++++++++++----------------------
 2 files changed, 45 insertions(+), 32 deletions(-)

[Qemu-devel] [PATCH 1/3] cleanup: bdrv_snaphost_find() returns zero or -ENOENT

[Miguel Di Ciurcio Filho]

The bdrv_snaphost_find() returns zero in case it finds an snapshot or -ENOENT in
case it doesn't.

Checking returning values as >= zero doesn't make sense.

Signed-off-by: Miguel Di Ciurcio Filho <miguel.filho@gmail.com>
---
 savevm.c |    7 ++++---
 1 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/savevm.c b/savevm.c
index 7a1de3c..6c6adb0 100644
--- a/savevm.c
+++ b/savevm.c
@@ -1768,7 +1768,7 @@ static int del_existing_snapshots(Monitor *mon, const char *name)
     bs = NULL;
     while ((bs = bdrv_next(bs))) {
         if (bdrv_can_snapshot(bs) &&
-            bdrv_snapshot_find(bs, snapshot, name) >= 0)
+            bdrv_snapshot_find(bs, snapshot, name) == 0)
         {
             ret = bdrv_snapshot_delete(bs, name);
             if (ret < 0) {
@@ -1948,8 +1948,9 @@ int load_vmstate(const char *name)
 
     /* 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;
+    if ((ret == 0) && (sn.vm_state_size == 0)) {
+            return -EINVAL;
+    }
 
     /* restore the VM state */
     f = qemu_fopen_bdrv(bs, 0);
-- 
1.7.1

Re: [Qemu-devel] [PATCH 1/3] cleanup: bdrv_snaphost_find() returns zero or -ENOENT

[Markus Armbruster]

Miguel Di Ciurcio Filho <miguel.filho@gmail.com> writes:

> The bdrv_snaphost_find() returns zero in case it finds an snapshot or -ENOENT in
> case it doesn't.
>
> Checking returning values as >= zero doesn't make sense.

Debatable.  "RETVAL < 0" is an idiomatic check for error.  "RETVAL >= 0"
is merely its negation.

Anyway, I do prefer code like this

    ret = do_something();
    if (ret < 0) {
        handle error...
    }
    do more...

over

    ret = do_something();
    if (ret >= 0) {
        do more...
    }

>
> Signed-off-by: Miguel Di Ciurcio Filho <miguel.filho@gmail.com>
> ---
>  savevm.c |    7 ++++---
>  1 files changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/savevm.c b/savevm.c
> index 7a1de3c..6c6adb0 100644
> --- a/savevm.c
> +++ b/savevm.c
> @@ -1768,7 +1768,7 @@ static int del_existing_snapshots(Monitor *mon, const char *name)
>      bs = NULL;
>      while ((bs = bdrv_next(bs))) {
>          if (bdrv_can_snapshot(bs) &&
> -            bdrv_snapshot_find(bs, snapshot, name) >= 0)
> +            bdrv_snapshot_find(bs, snapshot, name) == 0)
>          {
>              ret = bdrv_snapshot_delete(bs, name);
>              if (ret < 0) {
> @@ -1948,8 +1948,9 @@ int load_vmstate(const char *name)
>  
>      /* 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;
> +    if ((ret == 0) && (sn.vm_state_size == 0)) {
> +            return -EINVAL;
> +    }
>  
>      /* restore the VM state */
>      f = qemu_fopen_bdrv(bs, 0);

I wonder what happens if bdrv_snapshot_find() fails.  Why is it safe to
ignore that and continue?


do_savevm() has another one:

        ret = bdrv_snapshot_find(bs, old_sn, name);
        if (ret >= 0) {
            pstrcpy(sn->name, sizeof(sn->name), old_sn->name);
            pstrcpy(sn->id_str, sizeof(sn->id_str), old_sn->id_str);
        } else {
            pstrcpy(sn->name, sizeof(sn->name), name);
        }

Re: [Qemu-devel] [PATCH 1/3] cleanup: bdrv_snaphost_find() returns zero or -ENOENT

[Luiz Capitulino]

On Fri, 30 Jul 2010 11:44:26 +0200
Markus Armbruster <armbru@redhat.com> wrote:

> Miguel Di Ciurcio Filho <miguel.filho@gmail.com> writes:
> 
> > The bdrv_snaphost_find() returns zero in case it finds an snapshot or -ENOENT in
> > case it doesn't.
> >
> > Checking returning values as >= zero doesn't make sense.
> 
> Debatable.  "RETVAL < 0" is an idiomatic check for error.  "RETVAL >= 0"
> is merely its negation.

True, but Miguel's change makes the code less confusing, IMHO. I'm always
under the impression that this kind of code is counting on impossible
things (ie. on RETVAL > 0).

Idioms don't have this problem, obviously.

> Anyway, I do prefer code like this
> 
>     ret = do_something();
>     if (ret < 0) {
>         handle error...
>     }
>     do more...
> 
> over
> 
>     ret = do_something();
>     if (ret >= 0) {
>         do more...
>     }

Agreed, not worth fixing now though.

> > Signed-off-by: Miguel Di Ciurcio Filho <miguel.filho@gmail.com>
> > ---
> >  savevm.c |    7 ++++---
> >  1 files changed, 4 insertions(+), 3 deletions(-)
> >
> > diff --git a/savevm.c b/savevm.c
> > index 7a1de3c..6c6adb0 100644
> > --- a/savevm.c
> > +++ b/savevm.c
> > @@ -1768,7 +1768,7 @@ static int del_existing_snapshots(Monitor *mon, const char *name)
> >      bs = NULL;
> >      while ((bs = bdrv_next(bs))) {
> >          if (bdrv_can_snapshot(bs) &&
> > -            bdrv_snapshot_find(bs, snapshot, name) >= 0)
> > +            bdrv_snapshot_find(bs, snapshot, name) == 0)
> >          {
> >              ret = bdrv_snapshot_delete(bs, name);
> >              if (ret < 0) {
> > @@ -1948,8 +1948,9 @@ int load_vmstate(const char *name)
> >  
> >      /* 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;
> > +    if ((ret == 0) && (sn.vm_state_size == 0)) {
> > +            return -EINVAL;
> > +    }
> >  
> >      /* restore the VM state */
> >      f = qemu_fopen_bdrv(bs, 0);
> 
> I wonder what happens if bdrv_snapshot_find() fails.  Why is it safe to
> ignore that and continue?

Good point, turns out I wondered the same and fixed it in an past
RFC series:

 http://lists.gnu.org/archive/html/qemu-devel/2010-04/msg01321.html

Miguel, I think it's worth going through that series and cherry picking
what makes sense. Of course that you don't have to worry about QError.

> 
> 
> do_savevm() has another one:
> 
>         ret = bdrv_snapshot_find(bs, old_sn, name);
>         if (ret >= 0) {
>             pstrcpy(sn->name, sizeof(sn->name), old_sn->name);
>             pstrcpy(sn->id_str, sizeof(sn->id_str), old_sn->id_str);
>         } else {
>             pstrcpy(sn->name, sizeof(sn->name), name);
>         }
> 

[Qemu-devel] [PATCH 2/3] cleanup: del_existing_snapshots() must return the upstream error code

[Miguel Di Ciurcio Filho]

Signed-off-by: Miguel Di Ciurcio Filho <miguel.filho@gmail.com>
---
 savevm.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/savevm.c b/savevm.c
index 6c6adb0..fb38e8a 100644
--- a/savevm.c
+++ b/savevm.c
@@ -1775,7 +1775,7 @@ static int del_existing_snapshots(Monitor *mon, const char *name)
                 monitor_printf(mon,
                                "Error while deleting snapshot on '%s'\n",
                                bdrv_get_device_name(bs));
-                return -1;
+                return ret;
             }
         }
     }
-- 
1.7.1

Re: [Qemu-devel] [PATCH 2/3] cleanup: del_existing_snapshots() must return the upstream error code

[Markus Armbruster]

Why?

I figure the next patch wants it, but if that's the reason, the commit
message should state it.

Re: [Qemu-devel] [PATCH 2/3] cleanup: del_existing_snapshots() must return the upstream error code

[Miguel Di Ciurcio Filho]

On Fri, Jul 30, 2010 at 6:45 AM, Markus Armbruster <armbru@redhat.com> wrote:
> Why?
>
> I figure the next patch wants it, but if that's the reason, the commit
> message should state it.
>

To better identify what happened and where, IMHO.

Re: [Qemu-devel] [PATCH 2/3] cleanup: del_existing_snapshots() must return the upstream error code

[Markus Armbruster]

Miguel Di Ciurcio Filho <miguel.filho@gmail.com> writes:

> On Fri, Jul 30, 2010 at 6:45 AM, Markus Armbruster <armbru@redhat.com> wrote:
>> Why?
>>
>> I figure the next patch wants it, but if that's the reason, the commit
>> message should state it.
>>
>
> To better identify what happened and where, IMHO.

To let the *next* patch do a better job.  That's not obvious from this
patch, so better state it.

[Qemu-devel] [PATCH 3/3] savevm: prevent snapshot overwriting and generate a default name

[Miguel Di Ciurcio Filho]

This patch address two issues.

1) When savevm is run using an previously saved snapshot id or name, it will
delete the original and create a new one, using the same id and name and not
prompting the user of what just happened.

This behaviour is not good, IMHO.

We add a '-f' parameter to savevm, to really force that to happen, in case the
user really wants to.

New behavior:
(qemu) savevm snap1
An snapshot named 'snap1' already exists

(qemu) savevm -f snap1

We do better error reporting in case '-f' is used too than before.

2) When savevm is run without a name or id, the name stays blank.

This is a first step to hide the internal id, because I don't see a reason to
expose this kind of internals to the user.

The new behavior is when savevm is run without parameters a name will be
created automaticaly, so the snapshot is accessible to the user without needing
the id when loadvm is run.

(qemu) savevm
(qemu) info snapshots
ID        TAG                 VM SIZE                DATE       VM CLOCK
1         vm-20100728134640      978K 2010-07-28 13:46:40   00:00:08.603

We use a name with the format 'vm-YYYYMMDDHHMMSS'.

TODO: I have no clue on how to create a timestamp string when using Windows.

Signed-off-by: Miguel Di Ciurcio Filho <miguel.filho@gmail.com>
---
 qemu-monitor.hx |    9 ++++---
 savevm.c        |   59 ++++++++++++++++++++++++++++++++----------------------
 2 files changed, 40 insertions(+), 28 deletions(-)

diff --git a/qemu-monitor.hx b/qemu-monitor.hx
index 9c27b31..94e8178 100644
--- a/qemu-monitor.hx
+++ b/qemu-monitor.hx
@@ -275,14 +275,15 @@ ETEXI
 
     {
         .name       = "savevm",
-        .args_type  = "name:s?",
-        .params     = "[tag|id]",
-        .help       = "save a VM snapshot. If no tag or id are provided, a new snapshot is created",
+        .args_type  = "force:-f,name:s?",
+        .params     = "[-f] [name]",
+        .help       = "save a VM snapshot. If no name is provided, a new one will be generated"
+		      "\n\t\t\t -f to overwrite an snapshot if it already exists",
         .mhandler.cmd = do_savevm,
     },
 
 STEXI
-@item savevm [@var{tag}|@var{id}]
+@item savevm [-f] [@var{tag}]
 @findex savevm
 Create a snapshot of the whole virtual machine. If @var{tag} is
 provided, it is used as human readable identifier. If there is already
diff --git a/savevm.c b/savevm.c
index fb38e8a..ae6f29f 100644
--- a/savevm.c
+++ b/savevm.c
@@ -1785,7 +1785,7 @@ static int del_existing_snapshots(Monitor *mon, const char *name)
 
 void do_savevm(Monitor *mon, const QDict *qdict)
 {
-    BlockDriverState *bs, *bs1;
+    BlockDriverState *bs_vm_state, *bs;
     QEMUSnapshotInfo sn1, *sn = &sn1, old_sn1, *old_sn = &old_sn1;
     int ret;
     QEMUFile *f;
@@ -1796,7 +1796,9 @@ void do_savevm(Monitor *mon, const QDict *qdict)
 #else
     struct timeval tv;
 #endif
+    struct tm tm;
     const char *name = qdict_get_try_str(qdict, "name");
+    int force = qdict_get_try_bool(qdict, "force", 0);
 
     /* Verify if there is a device that doesn't support snapshots and is writable */
     bs = NULL;
@@ -1813,8 +1815,8 @@ void do_savevm(Monitor *mon, const QDict *qdict)
         }
     }
 
-    bs = bdrv_snapshots();
-    if (!bs) {
+    bs_vm_state = bdrv_snapshots();
+    if (!bs_vm_state) {
         monitor_printf(mon, "No block device can accept snapshots\n");
         return;
     }
@@ -1825,15 +1827,6 @@ void do_savevm(Monitor *mon, const QDict *qdict)
     vm_stop(0);
 
     memset(sn, 0, sizeof(*sn));
-    if (name) {
-        ret = bdrv_snapshot_find(bs, old_sn, name);
-        if (ret >= 0) {
-            pstrcpy(sn->name, sizeof(sn->name), old_sn->name);
-            pstrcpy(sn->id_str, sizeof(sn->id_str), old_sn->id_str);
-        } else {
-            pstrcpy(sn->name, sizeof(sn->name), name);
-        }
-    }
 
     /* fill auxiliary fields */
 #ifdef _WIN32
@@ -1847,13 +1840,31 @@ void do_savevm(Monitor *mon, const QDict *qdict)
 #endif
     sn->vm_clock_nsec = qemu_get_clock(vm_clock);
 
-    /* Delete old snapshots of the same name */
-    if (name && del_existing_snapshots(mon, name) < 0) {
-        goto the_end;
+    if (name) {
+        ret = bdrv_snapshot_find(bs_vm_state, old_sn, name);
+        if (ret == 0) {
+            if (force) {
+                ret = del_existing_snapshots(mon, name);
+                if (ret < 0) {
+                    monitor_printf(mon, "Error deleting snapshot '%s', error: %d\n",
+                        name, ret);
+                    goto the_end;
+                }
+            } else {
+                monitor_printf(mon, "An snapshot named '%s' already exists\n", name);
+                goto the_end;
+            }
+        }
+
+        pstrcpy(sn->name, sizeof(sn->name), name);
+    } else {
+        /* TODO: handle Windows */
+        localtime_r(&tv.tv_sec, &tm);
+        strftime(sn->name, sizeof(sn->name), "vm-%Y%m%d%H%M%S", &tm);
     }
 
     /* save the VM state */
-    f = qemu_fopen_bdrv(bs, 1);
+    f = qemu_fopen_bdrv(bs_vm_state, 1);
     if (!f) {
         monitor_printf(mon, "Could not open VM state file\n");
         goto the_end;
@@ -1867,23 +1878,23 @@ void do_savevm(Monitor *mon, const QDict *qdict)
     }
 
     /* create the snapshots */
-
-    bs1 = NULL;
-    while ((bs1 = bdrv_next(bs1))) {
-        if (bdrv_can_snapshot(bs1)) {
+    bs = NULL;
+    while ((bs = bdrv_next(bs))) {
+        if (bdrv_can_snapshot(bs)) {
             /* Write VM state size only to the image that contains the state */
-            sn->vm_state_size = (bs == bs1 ? vm_state_size : 0);
-            ret = bdrv_snapshot_create(bs1, sn);
+            sn->vm_state_size = (bs_vm_state == bs ? vm_state_size : 0);
+            ret = bdrv_snapshot_create(bs, sn);
             if (ret < 0) {
                 monitor_printf(mon, "Error while creating snapshot on '%s'\n",
-                               bdrv_get_device_name(bs1));
+                               bdrv_get_device_name(bs));
             }
         }
     }
 
  the_end:
-    if (saved_vm_running)
+    if (saved_vm_running) {
         vm_start();
+    }
 }
 
 int load_vmstate(const char *name)
-- 
1.7.1

Re: [Qemu-devel] [PATCH 3/3] savevm: prevent snapshot overwriting and generate a default name

[Markus Armbruster]

Miguel Di Ciurcio Filho <miguel.filho@gmail.com> writes:

> This patch address two issues.
>
> 1) When savevm is run using an previously saved snapshot id or name, it will
> delete the original and create a new one, using the same id and name and not
> prompting the user of what just happened.
>
> This behaviour is not good, IMHO.

Debatable.

> We add a '-f' parameter to savevm, to really force that to happen, in case the
> user really wants to.

Incompatible change, looks like it'll break libvirt.  Doesn't mean we
can't do it ever, but right now may not be the best time.  Perhaps after
savevm & friends are fully functional in QMP.

> New behavior:
> (qemu) savevm snap1
> An snapshot named 'snap1' already exists
>
> (qemu) savevm -f snap1
>
> We do better error reporting in case '-f' is used too than before.
>
> 2) When savevm is run without a name or id, the name stays blank.
>
> This is a first step to hide the internal id, because I don't see a reason to
> expose this kind of internals to the user.

Huh?

> The new behavior is when savevm is run without parameters a name will be
> created automaticaly, so the snapshot is accessible to the user without needing
> the id when loadvm is run.

Aha, now it makes sense.  Suggest to swap the paragraphs.

> (qemu) savevm
> (qemu) info snapshots
> ID        TAG                 VM SIZE                DATE       VM CLOCK
> 1         vm-20100728134640      978K 2010-07-28 13:46:40   00:00:08.603
>
> We use a name with the format 'vm-YYYYMMDDHHMMSS'.
>
> TODO: I have no clue on how to create a timestamp string when using Windows.
>
> Signed-off-by: Miguel Di Ciurcio Filho <miguel.filho@gmail.com>
> ---
>  qemu-monitor.hx |    9 ++++---
>  savevm.c        |   59 ++++++++++++++++++++++++++++++++----------------------
>  2 files changed, 40 insertions(+), 28 deletions(-)
>
> diff --git a/qemu-monitor.hx b/qemu-monitor.hx
> index 9c27b31..94e8178 100644
> --- a/qemu-monitor.hx
> +++ b/qemu-monitor.hx
> @@ -275,14 +275,15 @@ ETEXI
>  
>      {
>          .name       = "savevm",
> -        .args_type  = "name:s?",
> -        .params     = "[tag|id]",
> -        .help       = "save a VM snapshot. If no tag or id are provided, a new snapshot is created",
> +        .args_type  = "force:-f,name:s?",
> +        .params     = "[-f] [name]",
> +        .help       = "save a VM snapshot. If no name is provided, a new one will be generated"
> +		      "\n\t\t\t -f to overwrite an snapshot if it already exists",
>          .mhandler.cmd = do_savevm,
>      },
>  
>  STEXI
> -@item savevm [@var{tag}|@var{id}]
> +@item savevm [-f] [@var{tag}]
>  @findex savevm
>  Create a snapshot of the whole virtual machine. If @var{tag} is
>  provided, it is used as human readable identifier. If there is already
> diff --git a/savevm.c b/savevm.c
> index fb38e8a..ae6f29f 100644
> --- a/savevm.c
> +++ b/savevm.c
> @@ -1785,7 +1785,7 @@ static int del_existing_snapshots(Monitor *mon, const char *name)
>  
>  void do_savevm(Monitor *mon, const QDict *qdict)
>  {
> -    BlockDriverState *bs, *bs1;
> +    BlockDriverState *bs_vm_state, *bs;

These renames make the patch harder to review.

>      QEMUSnapshotInfo sn1, *sn = &sn1, old_sn1, *old_sn = &old_sn1;
>      int ret;
>      QEMUFile *f;
> @@ -1796,7 +1796,9 @@ void do_savevm(Monitor *mon, const QDict *qdict)
>  #else
>      struct timeval tv;
>  #endif
> +    struct tm tm;
>      const char *name = qdict_get_try_str(qdict, "name");
> +    int force = qdict_get_try_bool(qdict, "force", 0);
>  
>      /* Verify if there is a device that doesn't support snapshots and is writable */
>      bs = NULL;
> @@ -1813,8 +1815,8 @@ void do_savevm(Monitor *mon, const QDict *qdict)
>          }
>      }
>  
> -    bs = bdrv_snapshots();
> -    if (!bs) {
> +    bs_vm_state = bdrv_snapshots();
> +    if (!bs_vm_state) {
>          monitor_printf(mon, "No block device can accept snapshots\n");
>          return;
>      }
> @@ -1825,15 +1827,6 @@ void do_savevm(Monitor *mon, const QDict *qdict)
>      vm_stop(0);
>  
>      memset(sn, 0, sizeof(*sn));
> -    if (name) {
> -        ret = bdrv_snapshot_find(bs, old_sn, name);
> -        if (ret >= 0) {
> -            pstrcpy(sn->name, sizeof(sn->name), old_sn->name);
> -            pstrcpy(sn->id_str, sizeof(sn->id_str), old_sn->id_str);
> -        } else {
> -            pstrcpy(sn->name, sizeof(sn->name), name);
> -        }
> -    }
>  
>      /* fill auxiliary fields */
>  #ifdef _WIN32
> @@ -1847,13 +1840,31 @@ void do_savevm(Monitor *mon, const QDict *qdict)
>  #endif
>      sn->vm_clock_nsec = qemu_get_clock(vm_clock);
>  
> -    /* Delete old snapshots of the same name */
> -    if (name && del_existing_snapshots(mon, name) < 0) {
> -        goto the_end;
> +    if (name) {
> +        ret = bdrv_snapshot_find(bs_vm_state, old_sn, name);
> +        if (ret == 0) {
> +            if (force) {
> +                ret = del_existing_snapshots(mon, name);
> +                if (ret < 0) {
> +                    monitor_printf(mon, "Error deleting snapshot '%s', error: %d\n",
> +                        name, ret);
> +                    goto the_end;
> +                }
> +            } else {
> +                monitor_printf(mon, "An snapshot named '%s' already exists\n", name);
> +                goto the_end;
> +            }
> +        }
> +
> +        pstrcpy(sn->name, sizeof(sn->name), name);
> +    } else {
> +        /* TODO: handle Windows */
> +        localtime_r(&tv.tv_sec, &tm);
> +        strftime(sn->name, sizeof(sn->name), "vm-%Y%m%d%H%M%S", &tm);
>      }
>  
>      /* save the VM state */
> -    f = qemu_fopen_bdrv(bs, 1);
> +    f = qemu_fopen_bdrv(bs_vm_state, 1);
>      if (!f) {
>          monitor_printf(mon, "Could not open VM state file\n");
>          goto the_end;
> @@ -1867,23 +1878,23 @@ void do_savevm(Monitor *mon, const QDict *qdict)
>      }
>  
>      /* create the snapshots */
> -
> -    bs1 = NULL;
> -    while ((bs1 = bdrv_next(bs1))) {
> -        if (bdrv_can_snapshot(bs1)) {
> +    bs = NULL;
> +    while ((bs = bdrv_next(bs))) {
> +        if (bdrv_can_snapshot(bs)) {
>              /* Write VM state size only to the image that contains the state */
> -            sn->vm_state_size = (bs == bs1 ? vm_state_size : 0);
> -            ret = bdrv_snapshot_create(bs1, sn);
> +            sn->vm_state_size = (bs_vm_state == bs ? vm_state_size : 0);
> +            ret = bdrv_snapshot_create(bs, sn);
>              if (ret < 0) {
>                  monitor_printf(mon, "Error while creating snapshot on '%s'\n",
> -                               bdrv_get_device_name(bs1));
> +                               bdrv_get_device_name(bs));
>              }
>          }
>      }
>  
>   the_end:
> -    if (saved_vm_running)
> +    if (saved_vm_running) {
>          vm_start();
> +    }

I don't like style changes mixed up with functional changes in the same
patch.  It's fine if you have to touch the code anyway, but here you
don't.

>  }
>  
>  int load_vmstate(const char *name)

Re: [Qemu-devel] [PATCH 3/3] savevm: prevent snapshot overwriting and generate a default name

[Luiz Capitulino]

On Fri, 30 Jul 2010 11:34:57 +0200
Markus Armbruster <armbru@redhat.com> wrote:

> Miguel Di Ciurcio Filho <miguel.filho@gmail.com> writes:
> 
> > This patch address two issues.
> >
> > 1) When savevm is run using an previously saved snapshot id or name, it will
> > delete the original and create a new one, using the same id and name and not
> > prompting the user of what just happened.
> >
> > This behaviour is not good, IMHO.
> 
> Debatable.

Automatically destroying previously saved data without any notice seems
a quite bad behavior to me.

> > We add a '-f' parameter to savevm, to really force that to happen, in case the
> > user really wants to.
> 
> Incompatible change, looks like it'll break libvirt.  Doesn't mean we
> can't do it ever, but right now may not be the best time.  Perhaps after
> savevm & friends are fully functional in QMP.

Chris, could you please check whether this impacts libvirt?

Re: [Qemu-devel] [PATCH 3/3] savevm: prevent snapshot overwriting and generate a default name

[Markus Armbruster]

Luiz Capitulino <lcapitulino@redhat.com> writes:

> On Fri, 30 Jul 2010 11:34:57 +0200
> Markus Armbruster <armbru@redhat.com> wrote:
>
>> Miguel Di Ciurcio Filho <miguel.filho@gmail.com> writes:
>> 
>> > This patch address two issues.
>> >
>> > 1) When savevm is run using an previously saved snapshot id or name, it will
>> > delete the original and create a new one, using the same id and name and not
>> > prompting the user of what just happened.
>> >
>> > This behaviour is not good, IMHO.
>> 
>> Debatable.
>
> Automatically destroying previously saved data without any notice seems
> a quite bad behavior to me.

See cp, mv ;)

[...]

Re: [Qemu-devel] [PATCH 3/3] savevm: prevent snapshot overwriting and generate a default name

[Luiz Capitulino]

On Fri, 30 Jul 2010 16:43:09 +0200
Markus Armbruster <armbru@redhat.com> wrote:

> Luiz Capitulino <lcapitulino@redhat.com> writes:
> 
> > On Fri, 30 Jul 2010 11:34:57 +0200
> > Markus Armbruster <armbru@redhat.com> wrote:
> >
> >> Miguel Di Ciurcio Filho <miguel.filho@gmail.com> writes:
> >> 
> >> > This patch address two issues.
> >> >
> >> > 1) When savevm is run using an previously saved snapshot id or name, it will
> >> > delete the original and create a new one, using the same id and name and not
> >> > prompting the user of what just happened.
> >> >
> >> > This behaviour is not good, IMHO.
> >> 
> >> Debatable.
> >
> > Automatically destroying previously saved data without any notice seems
> > a quite bad behavior to me.
> 
> See cp, mv ;)

Nah, unrelated because the name of the command imply that. A best comparison
would be lvcreate have such semantics.

Re: [Qemu-devel] [PATCH 3/3] savevm: prevent snapshot overwriting and generate a default name

[Chris Lalancette]

On 07/30/10 - 10:43:01AM, Luiz Capitulino wrote:
> On Fri, 30 Jul 2010 11:34:57 +0200
> Markus Armbruster <armbru@redhat.com> wrote:
> 
> > Miguel Di Ciurcio Filho <miguel.filho@gmail.com> writes:
> > 
> > > This patch address two issues.
> > >
> > > 1) When savevm is run using an previously saved snapshot id or name, it will
> > > delete the original and create a new one, using the same id and name and not
> > > prompting the user of what just happened.
> > >
> > > This behaviour is not good, IMHO.
> > 
> > Debatable.
> 
> Automatically destroying previously saved data without any notice seems
> a quite bad behavior to me.
> 
> > > We add a '-f' parameter to savevm, to really force that to happen, in case the
> > > user really wants to.
> > 
> > Incompatible change, looks like it'll break libvirt.  Doesn't mean we
> > can't do it ever, but right now may not be the best time.  Perhaps after
> > savevm & friends are fully functional in QMP.
> 
> Chris, could you please check whether this impacts libvirt?

Sorry for the delay here.

As far as libvirt is concerned, this won't break the functionality, just
change the semantics.  If you only ever do snapshots through libvirt, then
we can never run into this situation; libvirt prevents 2 snapshots from having
the same name.  Today, if you do a mixture of snapshots through the monitor and
snapshots through libvirt, and you name them the same, then libvirt *could*
silently override the old one.  After this patch, the savevm will fail (which
libvirt will gracefully handle.  In any case, it's a corner case that libvirt
will never intentionally put itself into, so either way is fine with me.

(I also tend to think that overwriting data without any notification isn't
very nice, but I also understand that this is a change in semantics)

-- 
Chris Lalancette

[Qemu-devel] Re: [PATCH 3/3] savevm: prevent snapshot overwriting and generate a default name

[Luiz Capitulino]

On Wed, 28 Jul 2010 16:30:24 -0300
Miguel Di Ciurcio Filho <miguel.filho@gmail.com> wrote:

> This patch address two issues.

Then it should be split in two.

> 
> 1) When savevm is run using an previously saved snapshot id or name, it will
> delete the original and create a new one, using the same id and name and not
> prompting the user of what just happened.
> 
> This behaviour is not good, IMHO.
> 
> We add a '-f' parameter to savevm, to really force that to happen, in case the
> user really wants to.
> 
> New behavior:
> (qemu) savevm snap1
> An snapshot named 'snap1' already exists
> 
> (qemu) savevm -f snap1
> 
> We do better error reporting in case '-f' is used too than before.
> 
> 2) When savevm is run without a name or id, the name stays blank.
> 
> This is a first step to hide the internal id, because I don't see a reason to
> expose this kind of internals to the user.
> 
> The new behavior is when savevm is run without parameters a name will be
> created automaticaly, so the snapshot is accessible to the user without needing
> the id when loadvm is run.
> 
> (qemu) savevm
> (qemu) info snapshots
> ID        TAG                 VM SIZE                DATE       VM CLOCK
> 1         vm-20100728134640      978K 2010-07-28 13:46:40   00:00:08.603
> 
> We use a name with the format 'vm-YYYYMMDDHHMMSS'.
> 
> TODO: I have no clue on how to create a timestamp string when using Windows.

So, what happens on windows?

Also, please, avoid making changes which are unrelated to the patch, like:

>   the_end:
> -    if (saved_vm_running)
> +    if (saved_vm_running) {
>          vm_start();
> +    }
>  }
>  
>  int load_vmstate(const char *name)

[Qemu-devel] Re: [PATCH 3/3] savevm: prevent snapshot overwriting and generate a default name

[Miguel Di Ciurcio Filho]

On Fri, Jul 30, 2010 at 10:39 AM, Luiz Capitulino
<lcapitulino@redhat.com> wrote:
>>
>> TODO: I have no clue on how to create a timestamp string when using Windows.
>
> So, what happens on windows?
>

I've found some code in bdrv_snapshot_dump() that formats a timestamp
when using Windows. I will use it as a basis.

> Also, please, avoid making changes which are unrelated to the patch, like:
>
>>   the_end:
>> -    if (saved_vm_running)
>> +    if (saved_vm_running) {
>>          vm_start();
>> +    }
>>  }
>>

Ack. Coding style cleanups must have a separate patch :-D

Regards,

Miguel

[Qemu-devel] Re: [PATCH 3/3] savevm: prevent snapshot overwriting and generate a default name

[Luiz Capitulino]

On Fri, 30 Jul 2010 11:42:44 -0300
Miguel Di Ciurcio Filho <miguel.filho@gmail.com> wrote:

> On Fri, Jul 30, 2010 at 10:39 AM, Luiz Capitulino
> <lcapitulino@redhat.com> wrote:
> >>
> >> TODO: I have no clue on how to create a timestamp string when using Windows.
> >
> > So, what happens on windows?
> >
> 
> I've found some code in bdrv_snapshot_dump() that formats a timestamp
> when using Windows. I will use it as a basis.

How hard would be for you to test it, we shouldn't commit untested code.