[Qemu-devel] [PATCH] blockdev: Refuse to open encrypted image unless paused

[Qemu-devel] [PATCH] blockdev: Refuse to open encrypted image unless paused

[Markus Armbruster]

Opening an encrypted image takes an additional step: setting the key.
Between open and the key set, the image must not be used.

We have some protection against accidental use in place: you can't
unpause a guest while we're missing keys.  You can, however, hot-plug
block devices lacking keys into a running guest just fine, or insert
media lacking keys.  In the latter case, notifying the guest of the
insert is delayed until the key is set, which may suffice to protect
at least some guests in common usage.

This patch makes the protection apply in more cases, in a rather
heavy-handed way: it doesn't let you open encrypted images unless
we're in a paused state.

It doesn't extend the protection to users other than the guest (block
jobs?).  Use of runstate_check() from block.c is disgusting.  Best I
can do right now.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 block.c                | 8 +++++++-
 stubs/Makefile.objs    | 1 +
 stubs/runstate-check.c | 6 ++++++
 3 files changed, 14 insertions(+), 1 deletion(-)
 create mode 100644 stubs/runstate-check.c

diff --git a/block.c b/block.c
index f1ef4b0..7604881 100644
--- a/block.c
+++ b/block.c
@@ -1388,12 +1388,18 @@ done:
         ret = -EINVAL;
         goto close_and_fail;
     }
-    QDECREF(options);
 
     if (!bdrv_key_required(bs)) {
         bdrv_dev_change_media_cb(bs, true);
+    } else if (!runstate_check(RUN_STATE_PRELAUNCH)
+            && !runstate_check(RUN_STATE_PAUSED)) { /* HACK */
+        error_setg(errp,
+                   "Guest must be stopped for opening of encrypted image");
+        ret = -EBUSY;
+        goto close_and_fail;
     }
 
+    QDECREF(options);
     *pbs = bs;
     return 0;
 
diff --git a/stubs/Makefile.objs b/stubs/Makefile.objs
index df3aa7a..09e7790 100644
--- a/stubs/Makefile.objs
+++ b/stubs/Makefile.objs
@@ -19,6 +19,7 @@ stub-obj-y += mon-protocol-event.o
 stub-obj-y += mon-set-error.o
 stub-obj-y += pci-drive-hot-add.o
 stub-obj-y += reset.o
+stub-obj-y += runstate-check.o
 stub-obj-y += set-fd-handler.o
 stub-obj-y += slirp.o
 stub-obj-y += sysbus.o
diff --git a/stubs/runstate-check.c b/stubs/runstate-check.c
new file mode 100644
index 0000000..bd2e375
--- /dev/null
+++ b/stubs/runstate-check.c
@@ -0,0 +1,6 @@
+#include "sysemu/sysemu.h"
+
+bool runstate_check(RunState state)
+{
+    return state == RUN_STATE_PRELAUNCH;
+}
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH] blockdev: Refuse to open encrypted image unless paused

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1426 bytes --]

On 03/12/2014 11:00 AM, Markus Armbruster wrote:
> Opening an encrypted image takes an additional step: setting the key.
> Between open and the key set, the image must not be used.
> 
> We have some protection against accidental use in place: you can't
> unpause a guest while we're missing keys.  You can, however, hot-plug
> block devices lacking keys into a running guest just fine, or insert
> media lacking keys.  In the latter case, notifying the guest of the
> insert is delayed until the key is set, which may suffice to protect
> at least some guests in common usage.
> 
> This patch makes the protection apply in more cases, in a rather
> heavy-handed way: it doesn't let you open encrypted images unless
> we're in a paused state.
> 
> It doesn't extend the protection to users other than the guest (block
> jobs?).  Use of runstate_check() from block.c is disgusting.  Best I
> can do right now.

Better than what we had before, and worth having in 2.0

> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  block.c                | 8 +++++++-
>  stubs/Makefile.objs    | 1 +
>  stubs/runstate-check.c | 6 ++++++
>  3 files changed, 14 insertions(+), 1 deletion(-)
>  create mode 100644 stubs/runstate-check.c
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH] blockdev: Refuse to open encrypted image unless paused

[Fam Zheng]

On Wed, 03/12 18:00, Markus Armbruster wrote:
> Opening an encrypted image takes an additional step: setting the key.
> Between open and the key set, the image must not be used.
> 
> We have some protection against accidental use in place: you can't
> unpause a guest while we're missing keys.  You can, however, hot-plug
> block devices lacking keys into a running guest just fine, or insert
> media lacking keys.  In the latter case, notifying the guest of the
> insert is delayed until the key is set, which may suffice to protect
> at least some guests in common usage.
> 
> This patch makes the protection apply in more cases, in a rather
> heavy-handed way: it doesn't let you open encrypted images unless
> we're in a paused state.
> 
> It doesn't extend the protection to users other than the guest (block
> jobs?).  Use of runstate_check() from block.c is disgusting.  Best I
> can do right now.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  block.c                | 8 +++++++-
>  stubs/Makefile.objs    | 1 +
>  stubs/runstate-check.c | 6 ++++++
>  3 files changed, 14 insertions(+), 1 deletion(-)
>  create mode 100644 stubs/runstate-check.c
> 
> diff --git a/block.c b/block.c
> index f1ef4b0..7604881 100644
> --- a/block.c
> +++ b/block.c
> @@ -1388,12 +1388,18 @@ done:
>          ret = -EINVAL;
>          goto close_and_fail;
>      }
> -    QDECREF(options);
>  
>      if (!bdrv_key_required(bs)) {
>          bdrv_dev_change_media_cb(bs, true);
> +    } else if (!runstate_check(RUN_STATE_PRELAUNCH)
> +            && !runstate_check(RUN_STATE_PAUSED)) { /* HACK */
> +        error_setg(errp,
> +                   "Guest must be stopped for opening of encrypted image");

Changing error message here breaks qemu-iotests 087.

Thanks,
Fam

> +        ret = -EBUSY;
> +        goto close_and_fail;
>      }
>  
> +    QDECREF(options);
>      *pbs = bs;
>      return 0;
>  
> diff --git a/stubs/Makefile.objs b/stubs/Makefile.objs
> index df3aa7a..09e7790 100644
> --- a/stubs/Makefile.objs
> +++ b/stubs/Makefile.objs
> @@ -19,6 +19,7 @@ stub-obj-y += mon-protocol-event.o
>  stub-obj-y += mon-set-error.o
>  stub-obj-y += pci-drive-hot-add.o
>  stub-obj-y += reset.o
> +stub-obj-y += runstate-check.o
>  stub-obj-y += set-fd-handler.o
>  stub-obj-y += slirp.o
>  stub-obj-y += sysbus.o
> diff --git a/stubs/runstate-check.c b/stubs/runstate-check.c
> new file mode 100644
> index 0000000..bd2e375
> --- /dev/null
> +++ b/stubs/runstate-check.c
> @@ -0,0 +1,6 @@
> +#include "sysemu/sysemu.h"
> +
> +bool runstate_check(RunState state)
> +{
> +    return state == RUN_STATE_PRELAUNCH;
> +}
> -- 
> 1.8.1.4
> 
> 

Re: [Qemu-devel] [PATCH] blockdev: Refuse to open encrypted image unless paused

[Markus Armbruster]

Fam Zheng <famz@redhat.com> writes:

> On Wed, 03/12 18:00, Markus Armbruster wrote:
>> Opening an encrypted image takes an additional step: setting the key.
>> Between open and the key set, the image must not be used.
>> 
>> We have some protection against accidental use in place: you can't
>> unpause a guest while we're missing keys.  You can, however, hot-plug
>> block devices lacking keys into a running guest just fine, or insert
>> media lacking keys.  In the latter case, notifying the guest of the
>> insert is delayed until the key is set, which may suffice to protect
>> at least some guests in common usage.
>> 
>> This patch makes the protection apply in more cases, in a rather
>> heavy-handed way: it doesn't let you open encrypted images unless
>> we're in a paused state.
>> 
>> It doesn't extend the protection to users other than the guest (block
>> jobs?).  Use of runstate_check() from block.c is disgusting.  Best I
>> can do right now.
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  block.c                | 8 +++++++-
>>  stubs/Makefile.objs    | 1 +
>>  stubs/runstate-check.c | 6 ++++++
>>  3 files changed, 14 insertions(+), 1 deletion(-)
>>  create mode 100644 stubs/runstate-check.c
>> 
>> diff --git a/block.c b/block.c
>> index f1ef4b0..7604881 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -1388,12 +1388,18 @@ done:
>>          ret = -EINVAL;
>>          goto close_and_fail;
>>      }
>> -    QDECREF(options);
>>  
>>      if (!bdrv_key_required(bs)) {
>>          bdrv_dev_change_media_cb(bs, true);
>> +    } else if (!runstate_check(RUN_STATE_PRELAUNCH)
>> +            && !runstate_check(RUN_STATE_PAUSED)) { /* HACK */
>> +        error_setg(errp,
>> +                   "Guest must be stopped for opening of encrypted image");
>
> Changing error message here breaks qemu-iotests 087.

Crap.  I'm on vacation until Monday, just checking in to shepherd this
patch...

On *master*, "make check-block" reports

    Not run: 016 052 059 064 070 077
    Failures: 085 087
    Failed 2 of 34 tests

What am I doing wrong?

Re: [Qemu-devel] [PATCH] blockdev: Refuse to open encrypted image unless paused

[Fam Zheng]

On Thu, 03/13 14:25, Markus Armbruster wrote:
> Fam Zheng <famz@redhat.com> writes:
> 
> > On Wed, 03/12 18:00, Markus Armbruster wrote:
> >> Opening an encrypted image takes an additional step: setting the key.
> >> Between open and the key set, the image must not be used.
> >> 
> >> We have some protection against accidental use in place: you can't
> >> unpause a guest while we're missing keys.  You can, however, hot-plug
> >> block devices lacking keys into a running guest just fine, or insert
> >> media lacking keys.  In the latter case, notifying the guest of the
> >> insert is delayed until the key is set, which may suffice to protect
> >> at least some guests in common usage.
> >> 
> >> This patch makes the protection apply in more cases, in a rather
> >> heavy-handed way: it doesn't let you open encrypted images unless
> >> we're in a paused state.
> >> 
> >> It doesn't extend the protection to users other than the guest (block
> >> jobs?).  Use of runstate_check() from block.c is disgusting.  Best I
> >> can do right now.
> >> 
> >> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> >> ---
> >>  block.c                | 8 +++++++-
> >>  stubs/Makefile.objs    | 1 +
> >>  stubs/runstate-check.c | 6 ++++++
> >>  3 files changed, 14 insertions(+), 1 deletion(-)
> >>  create mode 100644 stubs/runstate-check.c
> >> 
> >> diff --git a/block.c b/block.c
> >> index f1ef4b0..7604881 100644
> >> --- a/block.c
> >> +++ b/block.c
> >> @@ -1388,12 +1388,18 @@ done:
> >>          ret = -EINVAL;
> >>          goto close_and_fail;
> >>      }
> >> -    QDECREF(options);
> >>  
> >>      if (!bdrv_key_required(bs)) {
> >>          bdrv_dev_change_media_cb(bs, true);
> >> +    } else if (!runstate_check(RUN_STATE_PRELAUNCH)
> >> +            && !runstate_check(RUN_STATE_PAUSED)) { /* HACK */
> >> +        error_setg(errp,
> >> +                   "Guest must be stopped for opening of encrypted image");
> >
> > Changing error message here breaks qemu-iotests 087.
> 
> Crap.  I'm on vacation until Monday, just checking in to shepherd this
> patch...
> 
> On *master*, "make check-block" reports
> 
>     Not run: 016 052 059 064 070 077
>     Failures: 085 087
>     Failed 2 of 34 tests
> 

I didn't run it from "make check-block" (Stefan sent a patch to remove 085 and
087 from check-block.

Running command: TEST_DIR=/tmp/qemu-iotests QEMU_PROG=../../x86_64-softmmu/qemu-system-x86_64 QEMU_IMG_PROG=../../qemu-img QEMU_IO_PROG=../../qemu-io QEMU_NBD_PROG=../../qemu-nbd ./check -qcow2 -o "" 087
QEMU          -- ../../x86_64-softmmu/qemu-system-x86_64
QEMU_IMG      -- ../../qemu-img
QEMU_IO       -- ../../qemu-io
QEMU_NBD      -- ../../qemu-nbd
IMGFMT        -- qcow2 (compat=1.1)
IMGPROTO      -- file
PLATFORM      -- Linux/x86_64 T430 3.13.6-1-ARCH
SOCKET_SCM_HELPER --

087 0s ... - output mismatch (see 087.out.bad)
--- 087.out     2014-03-12 09:28:04.167060760 +0800
+++ 087.out.bad 2014-03-14 09:11:59.770326144 +0800
@@ -31,7 +31,7 @@
 Testing:
 QMP_VERSION
 {"return": {}}
-{"error": {"class": "GenericError", "desc": "blockdev-add doesn't support encrypted devices"}}
+{"error": {"class": "GenericError", "desc": "could not open disk image disk: Guest must be stopped for opening of encrypted image"}}
 {"return": {}}
 {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "SHUTDOWN"}
 {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "DEVICE_TRAY_MOVED", "data": {"device": "ide1-cd0", "tray-open": true}}
Failures: 087
Failed 1 of 1 tests

Thanks,
Fam

Re: [Qemu-devel] [PATCH] blockdev: Refuse to open encrypted image unless paused

[Markus Armbruster]

Fam Zheng <famz@redhat.com> writes:

> On Thu, 03/13 14:25, Markus Armbruster wrote:
>> Fam Zheng <famz@redhat.com> writes:
>> 
>> > On Wed, 03/12 18:00, Markus Armbruster wrote:
>> >> Opening an encrypted image takes an additional step: setting the key.
>> >> Between open and the key set, the image must not be used.
>> >> 
>> >> We have some protection against accidental use in place: you can't
>> >> unpause a guest while we're missing keys.  You can, however, hot-plug
>> >> block devices lacking keys into a running guest just fine, or insert
>> >> media lacking keys.  In the latter case, notifying the guest of the
>> >> insert is delayed until the key is set, which may suffice to protect
>> >> at least some guests in common usage.
>> >> 
>> >> This patch makes the protection apply in more cases, in a rather
>> >> heavy-handed way: it doesn't let you open encrypted images unless
>> >> we're in a paused state.
>> >> 
>> >> It doesn't extend the protection to users other than the guest (block
>> >> jobs?).  Use of runstate_check() from block.c is disgusting.  Best I
>> >> can do right now.
>> >> 
>> >> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> >> ---
>> >>  block.c                | 8 +++++++-
>> >>  stubs/Makefile.objs    | 1 +
>> >>  stubs/runstate-check.c | 6 ++++++
>> >>  3 files changed, 14 insertions(+), 1 deletion(-)
>> >>  create mode 100644 stubs/runstate-check.c
>> >> 
>> >> diff --git a/block.c b/block.c
>> >> index f1ef4b0..7604881 100644
>> >> --- a/block.c
>> >> +++ b/block.c
>> >> @@ -1388,12 +1388,18 @@ done:
>> >>          ret = -EINVAL;
>> >>          goto close_and_fail;
>> >>      }
>> >> -    QDECREF(options);
>> >>  
>> >>      if (!bdrv_key_required(bs)) {
>> >>          bdrv_dev_change_media_cb(bs, true);
>> >> +    } else if (!runstate_check(RUN_STATE_PRELAUNCH)
>> >> +            && !runstate_check(RUN_STATE_PAUSED)) { /* HACK */
>> >> +        error_setg(errp,
>> >> +                   "Guest must be stopped for opening of encrypted image");
>> >
>> > Changing error message here breaks qemu-iotests 087.
>> 
>> Crap.  I'm on vacation until Monday, just checking in to shepherd this
>> patch...
>> 
>> On *master*, "make check-block" reports
>> 
>>     Not run: 016 052 059 064 070 077
>>     Failures: 085 087
>>     Failed 2 of 34 tests
>> 
>
> I didn't run it from "make check-block" (Stefan sent a patch to remove 085 and
> 087 from check-block.
>
> Running command: TEST_DIR=/tmp/qemu-iotests
> QEMU_PROG=../../x86_64-softmmu/qemu-system-x86_64
> QEMU_IMG_PROG=../../qemu-img QEMU_IO_PROG=../../qemu-io
> QEMU_NBD_PROG=../../qemu-nbd ./check -qcow2 -o "" 087
> QEMU          -- ../../x86_64-softmmu/qemu-system-x86_64
> QEMU_IMG      -- ../../qemu-img
> QEMU_IO       -- ../../qemu-io
> QEMU_NBD      -- ../../qemu-nbd
> IMGFMT        -- qcow2 (compat=1.1)
> IMGPROTO      -- file
> PLATFORM      -- Linux/x86_64 T430 3.13.6-1-ARCH
> SOCKET_SCM_HELPER --
>
> 087 0s ... - output mismatch (see 087.out.bad)
> --- 087.out     2014-03-12 09:28:04.167060760 +0800
> +++ 087.out.bad 2014-03-14 09:11:59.770326144 +0800
> @@ -31,7 +31,7 @@
>  Testing:
>  QMP_VERSION
>  {"return": {}}
> -{"error": {"class": "GenericError", "desc": "blockdev-add doesn't
> support encrypted devices"}}
> +{"error": {"class": "GenericError", "desc": "could not open disk
> image disk: Guest must be stopped for opening of encrypted image"}}
>  {"return": {}}
>  {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP},
> "event": "SHUTDOWN"}
>  {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP},
> "event": "DEVICE_TRAY_MOVED", "data": {"device": "ide1-cd0",
> "tray-open": true}}
> Failures: 087
> Failed 1 of 1 tests
>
> Thanks,
> Fam

I'll update this test to use -S, and add another one for the new failure
mode.  Thanks!

Re: [Qemu-devel] [PATCH] blockdev: Refuse to open encrypted image unless paused

[Paolo Bonzini]

Il 12/03/2014 18:00, Markus Armbruster ha scritto:
> +    } else if (!runstate_check(RUN_STATE_PRELAUNCH)
> +            && !runstate_check(RUN_STATE_PAUSED)) { /* HACK */

Why not "if (runstate_is_running())"?

Paolo

> +        error_setg(errp,
> +                   "Guest must be stopped for opening of encrypted image");
> +        ret = -EBUSY;
> +        goto close_and_fail;

Re: [Qemu-devel] [PATCH] blockdev: Refuse to open encrypted image unless paused

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> Il 12/03/2014 18:00, Markus Armbruster ha scritto:
>> +    } else if (!runstate_check(RUN_STATE_PRELAUNCH)
>> +            && !runstate_check(RUN_STATE_PAUSED)) { /* HACK */
>
> Why not "if (runstate_is_running())"?

The predicate actually wanted here is "monitor command 'cont' required
to get the guest running", because 'cont' is where the protection is.
My run state test is a crude approximation.

Re: [Qemu-devel] [PATCH] blockdev: Refuse to open encrypted image unless paused

[Paolo Bonzini]

Il 13/03/2014 14:18, Markus Armbruster ha scritto:
> Paolo Bonzini <pbonzini@redhat.com> writes:
> 
>> Il 12/03/2014 18:00, Markus Armbruster ha scritto:
>>> +    } else if (!runstate_check(RUN_STATE_PRELAUNCH)
>>> +            && !runstate_check(RUN_STATE_PAUSED)) { /* HACK */
>>
>> Why not "if (runstate_is_running())"?
> 
> The predicate actually wanted here is "monitor command 'cont' required
> to get the guest running", because 'cont' is where the protection is.
> My run state test is a crude approximation.
> 

Got it.  Then you need to add at least a check for
"runstate_check(RUN_STATE_INMIGRATE)", otherwise you break incoming
migration.  Actually, I think only SAVE_VM/RESTORE_VM/DEBUG are 
problematic, but I understand why you preferred a conservative
test (sufficient condition, not necessary).

You are singling out prelaunch and inmigrate because drive_init
will reset autostart to 0 for an encrypted image, right?

Paolo

Re: [Qemu-devel] [PATCH] blockdev: Refuse to open encrypted image unless paused

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> Il 13/03/2014 14:18, Markus Armbruster ha scritto:
>> Paolo Bonzini <pbonzini@redhat.com> writes:
>> 
>>> Il 12/03/2014 18:00, Markus Armbruster ha scritto:
>>>> +    } else if (!runstate_check(RUN_STATE_PRELAUNCH)
>>>> +            && !runstate_check(RUN_STATE_PAUSED)) { /* HACK */
>>>
>>> Why not "if (runstate_is_running())"?
>> 
>> The predicate actually wanted here is "monitor command 'cont' required
>> to get the guest running", because 'cont' is where the protection is.
>> My run state test is a crude approximation.
>> 
>
> Got it.  Then you need to add at least a check for
> "runstate_check(RUN_STATE_INMIGRATE)", otherwise you break incoming
> migration.

You're right: main() goes from RUN_STATE_PRELAUNCH to
RUN_STATE_INMIGRATE right when it sees -incoming.

>             Actually, I think only SAVE_VM/RESTORE_VM/DEBUG are 
> problematic, but I understand why you preferred a conservative
> test (sufficient condition, not necessary).

Exactly.

> You are singling out prelaunch and inmigrate because drive_init
> will reset autostart to 0 for an encrypted image, right?

Yes.

Re: [Qemu-devel] [PATCH] blockdev: Refuse to open encrypted image unless paused

[Paolo Bonzini]

Il 13/03/2014 16:00, Markus Armbruster ha scritto:
> Paolo Bonzini <pbonzini@redhat.com> writes:
>
>> Il 13/03/2014 14:18, Markus Armbruster ha scritto:
>>> Paolo Bonzini <pbonzini@redhat.com> writes:
>>>
>>>> Il 12/03/2014 18:00, Markus Armbruster ha scritto:
>>>>> +    } else if (!runstate_check(RUN_STATE_PRELAUNCH)
>>>>> +            && !runstate_check(RUN_STATE_PAUSED)) { /* HACK */
>>>>
>>>> Why not "if (runstate_is_running())"?
>>>
>>> The predicate actually wanted here is "monitor command 'cont' required
>>> to get the guest running", because 'cont' is where the protection is.
>>> My run state test is a crude approximation.
>>>
>>
>> Got it.  Then you need to add at least a check for
>> "runstate_check(RUN_STATE_INMIGRATE)", otherwise you break incoming
>> migration.
>
> You're right: main() goes from RUN_STATE_PRELAUNCH to
> RUN_STATE_INMIGRATE right when it sees -incoming.
>
>>             Actually, I think only SAVE_VM/RESTORE_VM/DEBUG are
>> problematic, but I understand why you preferred a conservative
>> test (sufficient condition, not necessary).
>
> Exactly.
>
>> You are singling out prelaunch and inmigrate because drive_init
>> will reset autostart to 0 for an encrypted image, right?
>
> Yes.

Then with the check modified,

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Paolo

Re: [Qemu-devel] [PATCH] blockdev: Refuse to open encrypted image unless paused

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> Il 13/03/2014 16:00, Markus Armbruster ha scritto:
>> Paolo Bonzini <pbonzini@redhat.com> writes:
>>
>>> Il 13/03/2014 14:18, Markus Armbruster ha scritto:
>>>> Paolo Bonzini <pbonzini@redhat.com> writes:
>>>>
>>>>> Il 12/03/2014 18:00, Markus Armbruster ha scritto:
>>>>>> +    } else if (!runstate_check(RUN_STATE_PRELAUNCH)
>>>>>> +            && !runstate_check(RUN_STATE_PAUSED)) { /* HACK */
>>>>>
>>>>> Why not "if (runstate_is_running())"?
>>>>
>>>> The predicate actually wanted here is "monitor command 'cont' required
>>>> to get the guest running", because 'cont' is where the protection is.
>>>> My run state test is a crude approximation.
>>>>
>>>
>>> Got it.  Then you need to add at least a check for
>>> "runstate_check(RUN_STATE_INMIGRATE)", otherwise you break incoming
>>> migration.
>>
>> You're right: main() goes from RUN_STATE_PRELAUNCH to
>> RUN_STATE_INMIGRATE right when it sees -incoming.
>>
>>>             Actually, I think only SAVE_VM/RESTORE_VM/DEBUG are
>>> problematic, but I understand why you preferred a conservative
>>> test (sufficient condition, not necessary).
>>
>> Exactly.
>>
>>> You are singling out prelaunch and inmigrate because drive_init
>>> will reset autostart to 0 for an encrypted image, right?
>>
>> Yes.
>
> Then with the check modified,
>
> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Will do.  I'm refraining from adding your R-by, because I need to update
tests, too.  Thanks!

Re: [Qemu-devel] [PATCH] blockdev: Refuse to open encrypted image unless paused

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 455 bytes --]

On 03/13/2014 04:43 AM, Paolo Bonzini wrote:
> Il 12/03/2014 18:00, Markus Armbruster ha scritto:
>> +    } else if (!runstate_check(RUN_STATE_PRELAUNCH)
>> +            && !runstate_check(RUN_STATE_PAUSED)) { /* HACK */
> 
> Why not "if (runstate_is_running())"?

Because that lacks PRELAUNCH, but PRELAUNCH also needs the protection.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH] blockdev: Refuse to open encrypted image unless paused

[Paolo Bonzini]

Il 13/03/2014 14:27, Eric Blake ha scritto:
>>> >> +    } else if (!runstate_check(RUN_STATE_PRELAUNCH)
>>> >> +            && !runstate_check(RUN_STATE_PAUSED)) { /* HACK */
>> >
>> > Why not "if (runstate_is_running())"?
> Because that lacks PRELAUNCH, but PRELAUNCH also needs the protection.

Nope, PRELAUNCH does *not* need the protection.

if (!runstate_check(PRELAUNCH) && !runstate_check(PAUSED)) error

	gives an error if runstate == anything *but*
	PRELAUNCH and PAUSED

It's at least DEBUG, SAVE_VM and RESTORE_VM that do need it but are not 
included in runstate_is_running.

Paolo