[Qemu-devel] [PATCH] add VMSTATE_BOOL

[Qemu-devel] [PATCH] add VMSTATE_BOOL

[Gerd Hoffmann]

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/hw.h  |   14 ++++++++++++++
 savevm.c |   21 +++++++++++++++++++++
 2 files changed, 35 insertions(+), 0 deletions(-)

diff --git a/hw/hw.h b/hw/hw.h
index e935364..234c713 100644
--- a/hw/hw.h
+++ b/hw/hw.h
@@ -333,6 +333,8 @@ struct VMStateDescription {
     const VMStateSubsection *subsections;
 };
 
+extern const VMStateInfo vmstate_info_bool;
+
 extern const VMStateInfo vmstate_info_int8;
 extern const VMStateInfo vmstate_info_int16;
 extern const VMStateInfo vmstate_info_int32;
@@ -613,6 +615,9 @@ extern const VMStateDescription vmstate_i2c_slave;
 #define VMSTATE_STRUCT_POINTER(_field, _state, _vmsd, _type)          \
     VMSTATE_STRUCT_POINTER_TEST(_field, _state, NULL, _vmsd, _type)
 
+#define VMSTATE_BOOL_V(_f, _s, _v)                                    \
+    VMSTATE_SINGLE(_f, _s, _v, vmstate_info_bool, bool)
+
 #define VMSTATE_INT8_V(_f, _s, _v)                                    \
     VMSTATE_SINGLE(_f, _s, _v, vmstate_info_int8, int8_t)
 #define VMSTATE_INT16_V(_f, _s, _v)                                   \
@@ -631,6 +636,9 @@ extern const VMStateDescription vmstate_i2c_slave;
 #define VMSTATE_UINT64_V(_f, _s, _v)                                  \
     VMSTATE_SINGLE(_f, _s, _v, vmstate_info_uint64, uint64_t)
 
+#define VMSTATE_BOOL(_f, _s)                                          \
+    VMSTATE_BOOL_V(_f, _s, 0)
+
 #define VMSTATE_INT8(_f, _s)                                          \
     VMSTATE_INT8_V(_f, _s, 0)
 #define VMSTATE_INT16(_f, _s)                                         \
@@ -685,6 +693,12 @@ extern const VMStateDescription vmstate_i2c_slave;
 #define VMSTATE_PTIMER(_f, _s)                                        \
     VMSTATE_PTIMER_V(_f, _s, 0)
 
+#define VMSTATE_BOOL_ARRAY_V(_f, _s, _n, _v)                         \
+    VMSTATE_ARRAY(_f, _s, _n, _v, vmstate_info_bool, bool)
+
+#define VMSTATE_BOOL_ARRAY(_f, _s, _n)                               \
+    VMSTATE_BOOL_ARRAY_V(_f, _s, _n, 0)
+
 #define VMSTATE_UINT16_ARRAY_V(_f, _s, _n, _v)                         \
     VMSTATE_ARRAY(_f, _s, _n, _v, vmstate_info_uint16, uint16_t)
 
diff --git a/savevm.c b/savevm.c
index 10057f3..14268ea 100644
--- a/savevm.c
+++ b/savevm.c
@@ -675,6 +675,27 @@ uint64_t qemu_get_be64(QEMUFile *f)
     return v;
 }
 
+/* bool */
+
+static int get_bool(QEMUFile *f, void *pv, size_t size)
+{
+    bool *v = pv;
+    *v = qemu_get_byte(f);
+    return 0;
+}
+
+static void put_bool(QEMUFile *f, void *pv, size_t size)
+{
+    bool *v = pv;
+    qemu_put_byte(f, *v);
+}
+
+const VMStateInfo vmstate_info_bool = {
+    .name = "bool",
+    .get  = get_bool,
+    .put  = put_bool,
+};
+
 /* 8 bit int */
 
 static int get_int8(QEMUFile *f, void *pv, size_t size)
-- 
1.7.1

[Qemu-devel] Re: [PATCH] add VMSTATE_BOOL

[malc]

On Mon, 1 Nov 2010, Gerd Hoffmann wrote:

> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  hw/hw.h  |   14 ++++++++++++++
>  savevm.c |   21 +++++++++++++++++++++
>  2 files changed, 35 insertions(+), 0 deletions(-)

Applied, thanks.

[..snip..]

-- 
mailto:av1474@comtv.ru

[Qemu-devel] Re: [PATCH] add VMSTATE_BOOL

[Michael S. Tsirkin]

On Mon, Nov 01, 2010 at 03:51:54PM +0100, Gerd Hoffmann wrote:
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  hw/hw.h  |   14 ++++++++++++++
>  savevm.c |   21 +++++++++++++++++++++
>  2 files changed, 35 insertions(+), 0 deletions(-)
> 
> diff --git a/hw/hw.h b/hw/hw.h
> index e935364..234c713 100644
> --- a/hw/hw.h
> +++ b/hw/hw.h
> @@ -333,6 +333,8 @@ struct VMStateDescription {
>      const VMStateSubsection *subsections;
>  };
>  
> +extern const VMStateInfo vmstate_info_bool;
> +
>  extern const VMStateInfo vmstate_info_int8;
>  extern const VMStateInfo vmstate_info_int16;
>  extern const VMStateInfo vmstate_info_int32;
> @@ -613,6 +615,9 @@ extern const VMStateDescription vmstate_i2c_slave;
>  #define VMSTATE_STRUCT_POINTER(_field, _state, _vmsd, _type)          \
>      VMSTATE_STRUCT_POINTER_TEST(_field, _state, NULL, _vmsd, _type)
>  
> +#define VMSTATE_BOOL_V(_f, _s, _v)                                    \
> +    VMSTATE_SINGLE(_f, _s, _v, vmstate_info_bool, bool)
> +
>  #define VMSTATE_INT8_V(_f, _s, _v)                                    \
>      VMSTATE_SINGLE(_f, _s, _v, vmstate_info_int8, int8_t)
>  #define VMSTATE_INT16_V(_f, _s, _v)                                   \
> @@ -631,6 +636,9 @@ extern const VMStateDescription vmstate_i2c_slave;
>  #define VMSTATE_UINT64_V(_f, _s, _v)                                  \
>      VMSTATE_SINGLE(_f, _s, _v, vmstate_info_uint64, uint64_t)
>  
> +#define VMSTATE_BOOL(_f, _s)                                          \
> +    VMSTATE_BOOL_V(_f, _s, 0)
> +
>  #define VMSTATE_INT8(_f, _s)                                          \
>      VMSTATE_INT8_V(_f, _s, 0)
>  #define VMSTATE_INT16(_f, _s)                                         \
> @@ -685,6 +693,12 @@ extern const VMStateDescription vmstate_i2c_slave;
>  #define VMSTATE_PTIMER(_f, _s)                                        \
>      VMSTATE_PTIMER_V(_f, _s, 0)
>  
> +#define VMSTATE_BOOL_ARRAY_V(_f, _s, _n, _v)                         \
> +    VMSTATE_ARRAY(_f, _s, _n, _v, vmstate_info_bool, bool)
> +
> +#define VMSTATE_BOOL_ARRAY(_f, _s, _n)                               \
> +    VMSTATE_BOOL_ARRAY_V(_f, _s, _n, 0)
> +

Why don't we pack the bits?

>  #define VMSTATE_UINT16_ARRAY_V(_f, _s, _n, _v)                         \
>      VMSTATE_ARRAY(_f, _s, _n, _v, vmstate_info_uint16, uint16_t)
>  
> diff --git a/savevm.c b/savevm.c
> index 10057f3..14268ea 100644
> --- a/savevm.c
> +++ b/savevm.c
> @@ -675,6 +675,27 @@ uint64_t qemu_get_be64(QEMUFile *f)
>      return v;
>  }
>  
> +/* bool */
> +
> +static int get_bool(QEMUFile *f, void *pv, size_t size)
> +{
> +    bool *v = pv;
> +    *v = qemu_get_byte(f);
> +    return 0;

We must really validate that the value is 0 or 1.
If it's not, we will get undefined behaviour.

> +}
> +
> +static void put_bool(QEMUFile *f, void *pv, size_t size)
> +{
> +    bool *v = pv;
> +    qemu_put_byte(f, *v);

Is there a guarantee that bool is a single byte, BTW?

> +}
> +
> +const VMStateInfo vmstate_info_bool = {
> +    .name = "bool",
> +    .get  = get_bool,
> +    .put  = put_bool,
> +};
> +
>  /* 8 bit int */
>  
>  static int get_int8(QEMUFile *f, void *pv, size_t size)
> -- 
> 1.7.1
> 

Re: [Qemu-devel] Re: [PATCH] add VMSTATE_BOOL

[Markus Armbruster]

"Michael S. Tsirkin" <mst@redhat.com> writes:

> On Mon, Nov 01, 2010 at 03:51:54PM +0100, Gerd Hoffmann wrote:
[...]
>> diff --git a/savevm.c b/savevm.c
>> index 10057f3..14268ea 100644
>> --- a/savevm.c
>> +++ b/savevm.c
>> @@ -675,6 +675,27 @@ uint64_t qemu_get_be64(QEMUFile *f)
>>      return v;
>>  }
>>  
>> +/* bool */
>> +
>> +static int get_bool(QEMUFile *f, void *pv, size_t size)
>> +{
>> +    bool *v = pv;
>> +    *v = qemu_get_byte(f);
>> +    return 0;
>
> We must really validate that the value is 0 or 1.
> If it's not, we will get undefined behaviour.

Indeed.

>> +}
>> +
>> +static void put_bool(QEMUFile *f, void *pv, size_t size)
>> +{
>> +    bool *v = pv;
>> +    qemu_put_byte(f, *v);
>
> Is there a guarantee that bool is a single byte, BTW?

Nope.  Does it matter?

[...]

[Qemu-devel] Re: [PATCH] add VMSTATE_BOOL

[Paolo Bonzini]

On 11/09/2010 10:23 AM, Markus Armbruster wrote:
>>> +}
>>> +
>>> +static void put_bool(QEMUFile *f, void *pv, size_t size)
>>> +{
>>> +    bool *v = pv;
>>> +    qemu_put_byte(f, *v);
>>
>> Is there a guarantee that bool is a single byte, BTW?
>
> Nope.  Does it matter?

No.  In fact I think QEMU makes (or tries to make) the QEMU dump format 
host-independent, and hence ABI-independent.  So, writing a single byte 
is better than writing sizeof(bool) bytes.

Paolo

[Qemu-devel] Re: [PATCH] add VMSTATE_BOOL

[Gerd Hoffmann]

   Hi,

>> +#define VMSTATE_BOOL_ARRAY(_f, _s, _n)                               \
>> +    VMSTATE_BOOL_ARRAY_V(_f, _s, _n, 0)
>> +
>
> Why don't we pack the bits?

Point being?  As long as we don't save *big* arrays of bools it simply 
isn't worth the effort IMHO.  And for big arrays we'll probably wouldn't 
use bool in the first place ...

>> +/* bool */
>> +
>> +static int get_bool(QEMUFile *f, void *pv, size_t size)
>> +{
>> +    bool *v = pv;
>> +    *v = qemu_get_byte(f);
>> +    return 0;
>
> We must really validate that the value is 0 or 1.
> If it's not, we will get undefined behaviour.

I disagree.

You indeed have a bug in case your bool ends up with a value being 
neither 0 nor 1.  That is completely independant from savevm/loadvm 
though, it can trip you up even in case you don't save/load the VM at all.

>> +}
>> +
>> +static void put_bool(QEMUFile *f, void *pv, size_t size)
>> +{
>> +    bool *v = pv;
>> +    qemu_put_byte(f, *v);
>
> Is there a guarantee that bool is a single byte, BTW?

No.  bool must be 0 or 1 though, and a single byte is big enough to keep 
that information.

cheers,
   Gerd

[Qemu-devel] Re: [PATCH] add VMSTATE_BOOL

[Michael S. Tsirkin]

On Tue, Nov 09, 2010 at 10:37:37AM +0100, Gerd Hoffmann wrote:
>   Hi,
> 
> >>+#define VMSTATE_BOOL_ARRAY(_f, _s, _n)                               \
> >>+    VMSTATE_BOOL_ARRAY_V(_f, _s, _n, 0)
> >>+
> >
> >Why don't we pack the bits?
> 
> Point being?  As long as we don't save *big* arrays of bools it
> simply isn't worth the effort IMHO.  And for big arrays we'll
> probably wouldn't use bool in the first place ...
> 
> >>+/* bool */
> >>+
> >>+static int get_bool(QEMUFile *f, void *pv, size_t size)
> >>+{
> >>+    bool *v = pv;
> >>+    *v = qemu_get_byte(f);
> >>+    return 0;
> >
> >We must really validate that the value is 0 or 1.
> >If it's not, we will get undefined behaviour.
> 
> I disagree.
> 
> You indeed have a bug in case your bool ends up with a value being
> neither 0 nor 1.  That is completely independant from savevm/loadvm
> though, it can trip you up even in case you don't save/load the VM
> at all.

I was wrong about undefined behaviour. Sorry.
What this implementation does is treat byte value '\0'
as boolean false, any other value as true.

I think we should verify that value is 0 or 1 and fail
migration otherwise, to make it more robust.


> >>+}
> >>+
> >>+static void put_bool(QEMUFile *f, void *pv, size_t size)
> >>+{
> >>+    bool *v = pv;
> >>+    qemu_put_byte(f, *v);
> >
> >Is there a guarantee that bool is a single byte, BTW?
> 
> No.  bool must be 0 or 1 though, and a single byte is big enough to
> keep that information.
> 
> cheers,
>   Gerd

Right.

[Qemu-devel] Re: [PATCH] add VMSTATE_BOOL

[Gerd Hoffmann]

   Hi,

>>>> +static int get_bool(QEMUFile *f, void *pv, size_t size)
>>>> +{
>>>> +    bool *v = pv;
>>>> +    *v = qemu_get_byte(f);
>>>> +    return 0;

> I think we should verify that value is 0 or 1 and fail
> migration otherwise, to make it more robust.

I still think such a check doesn't belong into the migration code as 
such a bug would exist without migration too.  And if anything we should 
check on save not on load, otherwise qemu can write out savevm images 
which it will refuse to load.  I wouldn't call this "robust".

cheers,
   Gerd

[Qemu-devel] Re: [PATCH] add VMSTATE_BOOL

[Michael S. Tsirkin]

On Tue, Nov 09, 2010 at 12:50:11PM +0100, Gerd Hoffmann wrote:
>   Hi,
> 
> >>>>+static int get_bool(QEMUFile *f, void *pv, size_t size)
> >>>>+{
> >>>>+    bool *v = pv;
> >>>>+    *v = qemu_get_byte(f);
> >>>>+    return 0;
> 
> >I think we should verify that value is 0 or 1 and fail
> >migration otherwise, to make it more robust.
> 
> I still think such a check doesn't belong into the migration code as
> such a bug would exist without migration too.  And if anything we
> should check on save not on load, otherwise qemu can write out
> savevm images which it will refuse to load.  I wouldn't call this
> "robust".
> 
> cheers,
>   Gerd

I think we should verify on load: e.g. the image could have
got corrupted. What, exactly, do you want to check on save?

---

savevm: validate bool values on load

We always save 0 or 1 values for booleans.  Validate on input to
increase the chance of detecting input corruption.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

---

diff --git a/savevm.c b/savevm.c
index 4e49765..da2fdfa 100644
--- a/savevm.c
+++ b/savevm.c
@@ -680,7 +680,12 @@ uint64_t qemu_get_be64(QEMUFile *f)
 static int get_bool(QEMUFile *f, void *pv, size_t size)
 {
     bool *v = pv;
-    *v = qemu_get_byte(f);
+    uint8_t b;
+    b = qemu_get_byte(f);
+    if (b != (uint8_t)true && b != (uint8_t)false) {
+        return -EINVAL;
+    }
+    *v = b;
     return 0;
 }
 

[Qemu-devel] Re: [PATCH] add VMSTATE_BOOL

[Gerd Hoffmann]

On 11/09/10 14:05, Michael S. Tsirkin wrote:
> On Tue, Nov 09, 2010 at 12:50:11PM +0100, Gerd Hoffmann wrote:
>>    Hi,
>>
>>>>>> +static int get_bool(QEMUFile *f, void *pv, size_t size)
>>>>>> +{
>>>>>> +    bool *v = pv;
>>>>>> +    *v = qemu_get_byte(f);
>>>>>> +    return 0;
>>
>>> I think we should verify that value is 0 or 1 and fail
>>> migration otherwise, to make it more robust.
>>
>> I still think such a check doesn't belong into the migration code as
>> such a bug would exist without migration too.  And if anything we
>> should check on save not on load, otherwise qemu can write out
>> savevm images which it will refuse to load.  I wouldn't call this
>> "robust".
>>
>> cheers,
>>    Gerd
>
> I think we should verify on load: e.g. the image could have
> got corrupted.

For catching corruption checksums work much better.

> What, exactly, do you want to check on save?

I don't want to check anything.

I'm just saying that *if* we are sanity-checking bool to catch bugs it 
is much more useful to do that when saving.

> savevm: validate bool values on load
>
> We always save 0 or 1 values for booleans.  Validate on input to
> increase the chance of detecting input corruption.

NACK.

cheers,
   Gerd

[Qemu-devel] Re: [PATCH] add VMSTATE_BOOL

[Michael S. Tsirkin]

On Tue, Nov 09, 2010 at 02:28:37PM +0100, Gerd Hoffmann wrote:
> On 11/09/10 14:05, Michael S. Tsirkin wrote:
> >On Tue, Nov 09, 2010 at 12:50:11PM +0100, Gerd Hoffmann wrote:
> >>   Hi,
> >>
> >>>>>>+static int get_bool(QEMUFile *f, void *pv, size_t size)
> >>>>>>+{
> >>>>>>+    bool *v = pv;
> >>>>>>+    *v = qemu_get_byte(f);
> >>>>>>+    return 0;
> >>
> >>>I think we should verify that value is 0 or 1 and fail
> >>>migration otherwise, to make it more robust.
> >>
> >>I still think such a check doesn't belong into the migration code as
> >>such a bug would exist without migration too.  And if anything we
> >>should check on save not on load, otherwise qemu can write out
> >>savevm images which it will refuse to load.  I wouldn't call this
> >>"robust".
> >>
> >>cheers,
> >>   Gerd
> >
> >I think we should verify on load: e.g. the image could have
> >got corrupted.
> 
> For catching corruption checksums work much better.

Unless there's a bug in software that writes the file, then checksum
will match.

> >What, exactly, do you want to check on save?
> 
> I don't want to check anything.

Why did you suggest it above then?

> I'm just saying that *if* we are sanity-checking bool

My patch doesn't check bool. Look at it. I am sanity
checking a byte read from file. File can have any values,
there is no guarantee that it has the same value
that the same version of qemu wrote out.

> to catch bugs
> it is much more useful to do that when saving.

There's nothing we *can* check.
	if (v == true || v == false)
is always true according to the language standard.
How is it useful to stick always true conditions that compiler
will likely remove in code?

> >savevm: validate bool values on load
> >
> >We always save 0 or 1 values for booleans.  Validate on input to
> >increase the chance of detecting input corruption.
> 
> NACK.
> 
> cheers,
>   Gerd