| Message ID | 1288623114-14439-1-git-send-email-kraxel@redhat.com |
|---|---|
| State | New |
| Headers | show |
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..]
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 >
"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? [...]
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
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
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.
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
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)
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> --- hw/hw.h | 14 ++++++++++++++ savevm.c | 21 +++++++++++++++++++++ 2 files changed, 35 insertions(+), 0 deletions(-)