[Qemu-devel] New option for snapshot_blkdev to avoid image creation

[Qemu-devel] New option for snapshot_blkdev to avoid image creation

[Federico Simoncelli]

In some situations might be useful to let qemu use an image that was
prepared for a live snapshot.
The advantage is that creating the snapshot file outside of the qemu
process we can use the whole range of options provided by the format
(eg for qcow2: encryption, cluster_size and preallocation).
It is also possible to pre-set a relative path to the backing file
(now it is created by default as absolute path).
In the long run it can also avoid the danger of reimplementing qemu-img
inside qemu (if we wanted to expose such options when a snapshot is
requested).

-- 
Federico.

[Qemu-devel] [PATCH] qemu: new option for snapshot_blkdev to avoid image creation

[Federico Simoncelli]

Add the new option [-n] for snapshot_blkdev to avoid the image creation.
The file provided as [new-image-file] is considered as already initialized
and will be used after passing a check for the backing file.

Signed-off-by: Federico Simoncelli <fsimonce@redhat.com>
---
 blockdev.c      |   54 ++++++++++++++++++++++++++++++++++++++++++++++++++++--
 hmp-commands.hx |    7 ++++---
 qmp-commands.hx |    4 ++--
 3 files changed, 58 insertions(+), 7 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 0827bf7..bd46808 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -550,8 +550,53 @@ void do_commit(Monitor *mon, const QDict *qdict)
     }
 }
 
+static int check_snapshot_file(const char *filename, const char *oldfilename,
+                               int flags, BlockDriver *drv)
+{
+    BlockDriverState *bs;
+    char bak_filename[1024], *abs_filename;
+    int ret = 0;
+
+    bs = bdrv_new("");
+    if (!bs) {
+        return -1;
+    }
+
+    ret = bdrv_open(bs, filename, flags, drv);
+    if (ret) {
+        qerror_report(QERR_OPEN_FILE_FAILED, filename);
+        goto err0;
+    }
+
+    if (bs->backing_file) {
+        path_combine(bak_filename, sizeof(bak_filename),
+                     filename, bs->backing_file);
+
+        abs_filename = realpath(bak_filename, NULL);
+        if (!abs_filename) {
+            ret = -1;
+            goto err1;
+        }
+
+        if (strcmp(abs_filename, oldfilename)) {
+            qerror_report(QERR_OPEN_FILE_FAILED, filename);
+            ret = -1;
+        }
+
+        free(abs_filename);
+    }
+
+err1:
+    bdrv_close(bs);
+
+err0:
+    bdrv_delete(bs);
+    return ret;
+}
+
 int do_snapshot_blkdev(Monitor *mon, const QDict *qdict, QObject **ret_data)
 {
+    const int nocreate = qdict_get_try_bool(qdict, "nocreate", 0);
     const char *device = qdict_get_str(qdict, "device");
     const char *filename = qdict_get_try_str(qdict, "snapshot-file");
     const char *format = qdict_get_try_str(qdict, "format");
@@ -597,8 +642,13 @@ int do_snapshot_blkdev(Monitor *mon, const QDict *qdict, QObject **ret_data)
         goto out;
     }
 
-    ret = bdrv_img_create(filename, format, bs->filename,
-                          bs->drv->format_name, NULL, -1, flags);
+    if (nocreate) {
+        ret = check_snapshot_file(filename, bs->filename, flags, drv);
+    } else {
+        ret = bdrv_img_create(filename, format, bs->filename,
+                              bs->drv->format_name, NULL, -1, flags);
+    }
+
     if (ret) {
         goto out;
     }
diff --git a/hmp-commands.hx b/hmp-commands.hx
index 9e1cca8..eb9fcd4 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -840,11 +840,12 @@ ETEXI
 
     {
         .name       = "snapshot_blkdev",
-        .args_type  = "device:B,snapshot-file:s?,format:s?",
-        .params     = "device [new-image-file] [format]",
+        .args_type  = "nocreate:-n,device:B,snapshot-file:s?,format:s?",
+        .params     = "[-n] device [new-image-file] [format]",
         .help       = "initiates a live snapshot\n\t\t\t"
                       "of device. If a new image file is specified, the\n\t\t\t"
-                      "new image file will become the new root image.\n\t\t\t"
+                      "new image file will be created (unless -n is\n\t\t\t"
+                      "specified) and will become the new root image.\n\t\t\t"
                       "If format is specified, the snapshot file will\n\t\t\t"
                       "be created in that format. Otherwise the\n\t\t\t"
                       "snapshot will be internal! (currently unsupported)",
diff --git a/qmp-commands.hx b/qmp-commands.hx
index d83bce5..7af36d8 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -695,8 +695,8 @@ EQMP
 
     {
         .name       = "blockdev-snapshot-sync",
-        .args_type  = "device:B,snapshot-file:s?,format:s?",
-        .params     = "device [new-image-file] [format]",
+        .args_type  = "nocreate:-n,device:B,snapshot-file:s?,format:s?",
+        .params     = "[-n] device [new-image-file] [format]",
         .user_print = monitor_user_noop,
         .mhandler.cmd_new = do_snapshot_blkdev,
     },
-- 
1.7.1

Re: [Qemu-devel] [PATCH] qemu: new option for snapshot_blkdev to avoid image creation

[Dor Laor]

On 10/03/2011 06:09 PM, Federico Simoncelli wrote:
> Add the new option [-n] for snapshot_blkdev to avoid the image creation.
> The file provided as [new-image-file] is considered as already initialized
> and will be used after passing a check for the backing file.

Seems ok to me as a way to go around fdget and still have selinux gain.
Worth to get Kevin's view too.

Federico, would you like to ack or extend the design:
http://wiki.qemu.org/Features/Snapshots

>
> Signed-off-by: Federico Simoncelli<fsimonce@redhat.com>
> ---
>   blockdev.c      |   54 ++++++++++++++++++++++++++++++++++++++++++++++++++++--
>   hmp-commands.hx |    7 ++++---
>   qmp-commands.hx |    4 ++--
>   3 files changed, 58 insertions(+), 7 deletions(-)
>
> diff --git a/blockdev.c b/blockdev.c
> index 0827bf7..bd46808 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -550,8 +550,53 @@ void do_commit(Monitor *mon, const QDict *qdict)
>       }
>   }
>
> +static int check_snapshot_file(const char *filename, const char *oldfilename,
> +                               int flags, BlockDriver *drv)
> +{
> +    BlockDriverState *bs;
> +    char bak_filename[1024], *abs_filename;
> +    int ret = 0;
> +
> +    bs = bdrv_new("");
> +    if (!bs) {
> +        return -1;
> +    }
> +
> +    ret = bdrv_open(bs, filename, flags, drv);
> +    if (ret) {
> +        qerror_report(QERR_OPEN_FILE_FAILED, filename);
> +        goto err0;
> +    }
> +
> +    if (bs->backing_file) {
> +        path_combine(bak_filename, sizeof(bak_filename),
> +                     filename, bs->backing_file);
> +
> +        abs_filename = realpath(bak_filename, NULL);
> +        if (!abs_filename) {
> +            ret = -1;
> +            goto err1;
> +        }
> +
> +        if (strcmp(abs_filename, oldfilename)) {
> +            qerror_report(QERR_OPEN_FILE_FAILED, filename);
> +            ret = -1;
> +        }
> +
> +        free(abs_filename);
> +    }
> +
> +err1:
> +    bdrv_close(bs);
> +
> +err0:
> +    bdrv_delete(bs);
> +    return ret;
> +}
> +
>   int do_snapshot_blkdev(Monitor *mon, const QDict *qdict, QObject **ret_data)
>   {
> +    const int nocreate = qdict_get_try_bool(qdict, "nocreate", 0);
>       const char *device = qdict_get_str(qdict, "device");
>       const char *filename = qdict_get_try_str(qdict, "snapshot-file");
>       const char *format = qdict_get_try_str(qdict, "format");
> @@ -597,8 +642,13 @@ int do_snapshot_blkdev(Monitor *mon, const QDict *qdict, QObject **ret_data)
>           goto out;
>       }
>
> -    ret = bdrv_img_create(filename, format, bs->filename,
> -                          bs->drv->format_name, NULL, -1, flags);
> +    if (nocreate) {
> +        ret = check_snapshot_file(filename, bs->filename, flags, drv);
> +    } else {
> +        ret = bdrv_img_create(filename, format, bs->filename,
> +                              bs->drv->format_name, NULL, -1, flags);
> +    }
> +
>       if (ret) {
>           goto out;
>       }
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index 9e1cca8..eb9fcd4 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -840,11 +840,12 @@ ETEXI
>
>       {
>           .name       = "snapshot_blkdev",
> -        .args_type  = "device:B,snapshot-file:s?,format:s?",
> -        .params     = "device [new-image-file] [format]",
> +        .args_type  = "nocreate:-n,device:B,snapshot-file:s?,format:s?",
> +        .params     = "[-n] device [new-image-file] [format]",
>           .help       = "initiates a live snapshot\n\t\t\t"
>                         "of device. If a new image file is specified, the\n\t\t\t"
> -                      "new image file will become the new root image.\n\t\t\t"
> +                      "new image file will be created (unless -n is\n\t\t\t"
> +                      "specified) and will become the new root image.\n\t\t\t"
>                         "If format is specified, the snapshot file will\n\t\t\t"
>                         "be created in that format. Otherwise the\n\t\t\t"
>                         "snapshot will be internal! (currently unsupported)",
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index d83bce5..7af36d8 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -695,8 +695,8 @@ EQMP
>
>       {
>           .name       = "blockdev-snapshot-sync",
> -        .args_type  = "device:B,snapshot-file:s?,format:s?",
> -        .params     = "device [new-image-file] [format]",
> +        .args_type  = "nocreate:-n,device:B,snapshot-file:s?,format:s?",
> +        .params     = "[-n] device [new-image-file] [format]",
>           .user_print = monitor_user_noop,
>           .mhandler.cmd_new = do_snapshot_blkdev,
>       },

Re: [Qemu-devel] [PATCH] qemu: new option for snapshot_blkdev to avoid image creation

[Federico Simoncelli]

----- Original Message -----
> From: "Dor Laor" <dlaor@redhat.com>
> To: "Federico Simoncelli" <fsimonce@redhat.com>
> Cc: qemu-devel@nongnu.org, abaron@redhat.com, "Kevin Wolf" <kwolf@redhat.com>
> Sent: Friday, October 7, 2011 12:45:06 AM
> Subject: Re: [Qemu-devel] [PATCH] qemu: new option for snapshot_blkdev to avoid image creation
> 
> On 10/03/2011 06:09 PM, Federico Simoncelli wrote:
> > Add the new option [-n] for snapshot_blkdev to avoid the image
> > creation.
> > The file provided as [new-image-file] is considered as already
> > initialized
> > and will be used after passing a check for the backing file.
> 
> Seems ok to me as a way to go around fdget and still have selinux
> gain.
> Worth to get Kevin's view too.

Hi Kevin, any news on this?

> Federico, would you like to ack or extend the design:
> http://wiki.qemu.org/Features/Snapshots

Dor, should I do it now or only after my patch is acked?

-- 
Federico

Re: [Qemu-devel] [PATCH] qemu: new option for snapshot_blkdev to avoid image creation

[Dor Laor]

On 10/11/2011 01:56 PM, Federico Simoncelli wrote:
> ----- Original Message -----
>> From: "Dor Laor"<dlaor@redhat.com>
>> To: "Federico Simoncelli"<fsimonce@redhat.com>
>> Cc: qemu-devel@nongnu.org, abaron@redhat.com, "Kevin Wolf"<kwolf@redhat.com>
>> Sent: Friday, October 7, 2011 12:45:06 AM
>> Subject: Re: [Qemu-devel] [PATCH] qemu: new option for snapshot_blkdev to avoid image creation
>>
>> On 10/03/2011 06:09 PM, Federico Simoncelli wrote:
>>> Add the new option [-n] for snapshot_blkdev to avoid the image
>>> creation.
>>> The file provided as [new-image-file] is considered as already
>>> initialized
>>> and will be used after passing a check for the backing file.
>>
>> Seems ok to me as a way to go around fdget and still have selinux
>> gain.
>> Worth to get Kevin's view too.
>
> Hi Kevin, any news on this?
>
>> Federico, would you like to ack or extend the design:
>> http://wiki.qemu.org/Features/Snapshots
>
> Dor, should I do it now or only after my patch is acked?

We're not that strict in the OSS world ;) you can do it before and mark 
as a pending request.
btw: Kevin is back so he will probably drain his queue soon.

Re: [Qemu-devel] New option for snapshot_blkdev to avoid image creation

[Stefan Hajnoczi]

On Mon, Oct 03, 2011 at 04:09:01PM +0000, Federico Simoncelli wrote:
> In some situations might be useful to let qemu use an image that was
> prepared for a live snapshot.
> The advantage is that creating the snapshot file outside of the qemu
> process we can use the whole range of options provided by the format
> (eg for qcow2: encryption, cluster_size and preallocation).
> It is also possible to pre-set a relative path to the backing file
> (now it is created by default as absolute path).
> In the long run it can also avoid the danger of reimplementing qemu-img
> inside qemu (if we wanted to expose such options when a snapshot is
> requested).

When the image file is created based on the backing file size:

$ qemu-img create -f qcow2 -o backing_file=master.img vm001.qcow2

It turns out that bdrv_img_create() opens the backing file with
read/write permissions.  This is generally a bad idea but especially
dangerous when the VM currently has the image file open already since
image formats are not designed for multiple initiators (clustering).  We
wouldn't want any caches being written out or startup fsck-style
operations to be performed on the backing file while the VM has it open.

Please make sure to use read-only before applying this patch.

Stefan

Re: [Qemu-devel] New option for snapshot_blkdev to avoid image creation

[Federico Simoncelli]

----- Original Message -----
> From: "Stefan Hajnoczi" <stefanha@gmail.com>
> To: "Federico Simoncelli" <fsimonce@redhat.com>
> Cc: qemu-devel@nongnu.org, abaron@redhat.com, dlaor@redhat.com
> Sent: Tuesday, October 4, 2011 9:33:48 AM
> Subject: Re: [Qemu-devel] New option for snapshot_blkdev to avoid image creation
> 
> On Mon, Oct 03, 2011 at 04:09:01PM +0000, Federico Simoncelli wrote:
> > In some situations might be useful to let qemu use an image that
> > was
> > prepared for a live snapshot.
> > The advantage is that creating the snapshot file outside of the
> > qemu
> > process we can use the whole range of options provided by the
> > format
> > (eg for qcow2: encryption, cluster_size and preallocation).
> > It is also possible to pre-set a relative path to the backing file
> > (now it is created by default as absolute path).
> > In the long run it can also avoid the danger of reimplementing
> > qemu-img
> > inside qemu (if we wanted to expose such options when a snapshot is
> > requested).
> 
> When the image file is created based on the backing file size:
> 
> $ qemu-img create -f qcow2 -o backing_file=master.img vm001.qcow2
> 
> It turns out that bdrv_img_create() opens the backing file with
> read/write permissions.  This is generally a bad idea but especially
> dangerous when the VM currently has the image file open already since
> image formats are not designed for multiple initiators (clustering).

Hi Stefan, are you sure that bdrv_img_create opens the backing file
with read/write permissions?
After a quick check I can't see that happening:

$ ./qemu-img create -f qcow2 /tmp/master.img 1G
Formatting '/tmp/master.img', fmt=qcow2 size=1073741824 encryption=off cluster_size=65536

$ strace -e trace=open ./qemu-img create -f qcow2 -o backing_file=/tmp/master.img /tmp/vm001.qcow2
[...]
open("/tmp/vm001.qcow2", O_RDONLY|O_NONBLOCK) = -1 ENOENT (No such file or directory)
open("/tmp/vm001.qcow2", O_RDONLY|O_NONBLOCK) = -1 ENOENT (No such file or directory)
open("/tmp/master.img", O_RDONLY|O_NONBLOCK) = 3
open("/tmp/master.img", O_RDONLY|O_NONBLOCK) = 3
open("/tmp/master.img", O_RDONLY|O_DSYNC|O_CLOEXEC) = 3
open("/tmp/master.img", O_RDONLY|O_NONBLOCK) = 3
open("/tmp/master.img", O_RDONLY|O_NONBLOCK) = 3
open("/tmp/master.img", O_RDONLY|O_CLOEXEC) = 3
Formatting '/tmp/vm001.qcow2', fmt=qcow2 size=1073741824 backing_file='/tmp/master.img' encryption=off cluster_size=65536 
open("/tmp/vm001.qcow2", O_RDONLY|O_NONBLOCK) = -1 ENOENT (No such file or directory)
open("/tmp/vm001.qcow2", O_RDONLY|O_NONBLOCK) = -1 ENOENT (No such file or directory)
open("/tmp/vm001.qcow2", O_WRONLY|O_CREAT|O_TRUNC, 0644) = 6
open("/tmp/vm001.qcow2", O_RDONLY|O_NONBLOCK) = 6
open("/tmp/vm001.qcow2", O_RDONLY|O_NONBLOCK) = 6
open("/tmp/vm001.qcow2", O_RDWR|O_DSYNC|O_CLOEXEC) = 6
open("/tmp/vm001.qcow2", O_RDONLY|O_NONBLOCK) = 6
open("/tmp/vm001.qcow2", O_RDONLY|O_NONBLOCK) = 6
open("/tmp/vm001.qcow2", O_RDWR|O_CLOEXEC) = 6

-- 
Federico

Re: [Qemu-devel] New option for snapshot_blkdev to avoid image creation

[Stefan Hajnoczi]

On Tue, Oct 04, 2011 at 04:27:42AM -0400, Federico Simoncelli wrote:
> ----- Original Message -----
> > From: "Stefan Hajnoczi" <stefanha@gmail.com>
> > To: "Federico Simoncelli" <fsimonce@redhat.com>
> > Cc: qemu-devel@nongnu.org, abaron@redhat.com, dlaor@redhat.com
> > Sent: Tuesday, October 4, 2011 9:33:48 AM
> > Subject: Re: [Qemu-devel] New option for snapshot_blkdev to avoid image creation
> > 
> > On Mon, Oct 03, 2011 at 04:09:01PM +0000, Federico Simoncelli wrote:
> > > In some situations might be useful to let qemu use an image that
> > > was
> > > prepared for a live snapshot.
> > > The advantage is that creating the snapshot file outside of the
> > > qemu
> > > process we can use the whole range of options provided by the
> > > format
> > > (eg for qcow2: encryption, cluster_size and preallocation).
> > > It is also possible to pre-set a relative path to the backing file
> > > (now it is created by default as absolute path).
> > > In the long run it can also avoid the danger of reimplementing
> > > qemu-img
> > > inside qemu (if we wanted to expose such options when a snapshot is
> > > requested).
> > 
> > When the image file is created based on the backing file size:
> > 
> > $ qemu-img create -f qcow2 -o backing_file=master.img vm001.qcow2
> > 
> > It turns out that bdrv_img_create() opens the backing file with
> > read/write permissions.  This is generally a bad idea but especially
> > dangerous when the VM currently has the image file open already since
> > image formats are not designed for multiple initiators (clustering).
> 
> Hi Stefan, are you sure that bdrv_img_create opens the backing file
> with read/write permissions?

You are absolutely right.  BDRV_O_FLAGS does not have BDRV_O_RDWR so it
is opening read-only.  I missed this when reading the code earlier
today.

Stefan

Re: [Qemu-devel] New option for snapshot_blkdev to avoid image creation

[Kevin Wolf]

Am 03.10.2011 18:09, schrieb Federico Simoncelli:
> In some situations might be useful to let qemu use an image that was
> prepared for a live snapshot.
> The advantage is that creating the snapshot file outside of the qemu
> process we can use the whole range of options provided by the format
> (eg for qcow2: encryption, cluster_size and preallocation).

In fact, I don't think encryption works, nor does preallocation work
with a backing file. I think you would have to check that the new image
file isn't encrypted, doesn't have any clusters allocated, etc.

I'm not sure that it makes sense to put the logic into an external
program (i.e. make it not available for users not using management
tools) when at the same you have to implement the same amount of checks
in qemu that verify that the management tool actually did what it is
supposed to.

> It is also possible to pre-set a relative path to the backing file
> (now it is created by default as absolute path).

Your code performs a literal string comparison with the old file name
used to open the image. So if it was relative on the qemu command line,
the backing file link must be relative as well, and if it was absolute
on the command line, it must be absolute.

> In the long run it can also avoid the danger of reimplementing qemu-img
> inside qemu (if we wanted to expose such options when a snapshot is
> requested).

This is not reimplementing, it is reusing.

bdrv_image_create() can do everything that qemu-img create can do.
qemu-img has only a thin wrapper around it. The only thing you need to
do is exposing the existing options in the monitor.

Kevin