Patchwork Re: [PATCH] add VMSTATE_BOOL

login
register
mail settings
Submitter Michael S. Tsirkin
Date Nov. 9, 2010, 1:05 p.m.
Message ID <20101109130530.GD22705@redhat.com>
Download mbox | patch
Permalink /patch/70541/
State New
Headers show

Comments

Michael S. Tsirkin - Nov. 9, 2010, 1:05 p.m.
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>

---
Gerd Hoffmann - Nov. 9, 2010, 1:28 p.m.
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
Michael S. Tsirkin - Nov. 9, 2010, 1:37 p.m.
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

Patch

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;
 }