Patchwork [07/11] eeprom93xx: Use the new hack macro to avoid duplicate field names

login
register
mail settings
Submitter Anthony Liguori
Date March 23, 2011, 12:16 a.m.
Message ID <1300839376-22520-8-git-send-email-aliguori@us.ibm.com>
Download mbox | patch
Permalink /patch/87980/
State New
Headers show

Comments

Anthony Liguori - March 23, 2011, 12:16 a.m.
I don't fully understand this hack business but we need field to be unique so..

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
 hw/eeprom93xx.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
Juan Quintela - March 23, 2011, 9:58 a.m.
Anthony Liguori <aliguori@us.ibm.com> wrote:
> I don't fully understand this hack business but we need field to be unique so..
>
> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> ---
>  hw/eeprom93xx.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/hw/eeprom93xx.c b/hw/eeprom93xx.c
> index cfa695d..f1d75ec 100644
> --- a/hw/eeprom93xx.c
> +++ b/hw/eeprom93xx.c
> @@ -114,7 +114,7 @@ static const VMStateInfo vmstate_hack_uint16_from_uint8 = {
>  };
>  
>  #define VMSTATE_UINT16_HACK_TEST(_f, _s, _t)                           \
> -    VMSTATE_SINGLE_TEST(_f, _s, _t, 0, vmstate_hack_uint16_from_uint8, uint16_t)
> +    VMSTATE_SINGLE_TEST_HACK(_f, _s, _t, 0, vmstate_hack_uint16_from_uint8, uint16_t)
>  
>  static bool is_old_eeprom_version(void *opaque, int version_id)
>  {

After the fact, we need to promote it as "full types".

Basically it is needed when we sent a field with a different size that
we use it on the struct.

if we have

struct FOOState {
       int32_t bar;
....
}

and it is sent as

VMSTATE_INT8(bar, ....)

In this case, I went through the whole device, checed that int8_t was
enough and did the change.

But if we have:

struct FOOState {
       int8_t bar;
....
}

and it is sent as

VMSTATE_INT32(bar, ....)

Then it is not trivial :-(

We change FOOState to int32 or we break migration format.  Here is where
the _HACK suffix appeared.

I thought it was not going to be needed a lot, but there are several
devices that just sent everything over the wire as uint32, independently
of its type.

Later, Juan.
Anthony Liguori - March 23, 2011, 12:34 p.m.
On 03/23/2011 04:58 AM, Juan Quintela wrote:
> Anthony Liguori<aliguori@us.ibm.com>  wrote:
>> I don't fully understand this hack business but we need field to be unique so..
>>
>> Signed-off-by: Anthony Liguori<aliguori@us.ibm.com>
>> ---
>>   hw/eeprom93xx.c |    2 +-
>>   1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/hw/eeprom93xx.c b/hw/eeprom93xx.c
>> index cfa695d..f1d75ec 100644
>> --- a/hw/eeprom93xx.c
>> +++ b/hw/eeprom93xx.c
>> @@ -114,7 +114,7 @@ static const VMStateInfo vmstate_hack_uint16_from_uint8 = {
>>   };
>>
>>   #define VMSTATE_UINT16_HACK_TEST(_f, _s, _t)                           \
>> -    VMSTATE_SINGLE_TEST(_f, _s, _t, 0, vmstate_hack_uint16_from_uint8, uint16_t)
>> +    VMSTATE_SINGLE_TEST_HACK(_f, _s, _t, 0, vmstate_hack_uint16_from_uint8, uint16_t)
>>
>>   static bool is_old_eeprom_version(void *opaque, int version_id)
>>   {
> After the fact, we need to promote it as "full types".
>
> Basically it is needed when we sent a field with a different size that
> we use it on the struct.
>
> if we have
>
> struct FOOState {
>         int32_t bar;
> ....
> }
>
> and it is sent as
>
> VMSTATE_INT8(bar, ....)
>
> In this case, I went through the whole device, checed that int8_t was
> enough and did the change.
>
> But if we have:
>
> struct FOOState {
>         int8_t bar;
> ....
> }
>
> and it is sent as
>
> VMSTATE_INT32(bar, ....)
>
> Then it is not trivial :-(
>
> We change FOOState to int32 or we break migration format.  Here is where
> the _HACK suffix appeared.
>
> I thought it was not going to be needed a lot, but there are several
> devices that just sent everything over the wire as uint32, independently
> of its type.

Could we get away with just doing:

VMSTATE_UNUSED(3),
VMSTATE_UINT8(bar, ...),

That's fully compatible on the wire and seems to be a clearer expression 
of exactly what the problem is.

Regards,

Anthony Liguori

> Later, Juan.
>
Juan Quintela - March 23, 2011, 2:14 p.m.
Anthony Liguori <anthony@codemonkey.ws> wrote:
> On 03/23/2011 04:58 AM, Juan Quintela wrote:
>> Anthony Liguori<aliguori@us.ibm.com>  wrote:
>>> I don't fully understand this hack business but we need field to be unique so..
>>>
>>> Signed-off-by: Anthony Liguori<aliguori@us.ibm.com>
>>> ---
>>>   hw/eeprom93xx.c |    2 +-
>>>   1 files changed, 1 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/hw/eeprom93xx.c b/hw/eeprom93xx.c
>>> index cfa695d..f1d75ec 100644
>>> --- a/hw/eeprom93xx.c
>>> +++ b/hw/eeprom93xx.c
>>> @@ -114,7 +114,7 @@ static const VMStateInfo vmstate_hack_uint16_from_uint8 = {
>>>   };
>>>
>>>   #define VMSTATE_UINT16_HACK_TEST(_f, _s, _t)                           \
>>> -    VMSTATE_SINGLE_TEST(_f, _s, _t, 0, vmstate_hack_uint16_from_uint8, uint16_t)
>>> +    VMSTATE_SINGLE_TEST_HACK(_f, _s, _t, 0, vmstate_hack_uint16_from_uint8, uint16_t)
>>>
>>>   static bool is_old_eeprom_version(void *opaque, int version_id)
>>>   {

> Could we get away with just doing:
>
> VMSTATE_UNUSED(3),
> VMSTATE_UINT8(bar, ...),

Remember that we are "supposed to be" big/little endian safe.

> That's fully compatible on the wire and seems to be a clearer
> expression of exactly what the problem is.

if we are going to break big endian machines, I fully agree.

Later, Juan.
Anthony Liguori - March 23, 2011, 2:33 p.m.
On 03/23/2011 09:14 AM, Juan Quintela wrote:
> Anthony Liguori<anthony@codemonkey.ws>  wrote:
>> On 03/23/2011 04:58 AM, Juan Quintela wrote:
>>> Anthony Liguori<aliguori@us.ibm.com>   wrote:
>>>> I don't fully understand this hack business but we need field to be unique so..
>>>>
>>>> Signed-off-by: Anthony Liguori<aliguori@us.ibm.com>
>>>> ---
>>>>    hw/eeprom93xx.c |    2 +-
>>>>    1 files changed, 1 insertions(+), 1 deletions(-)
>>>>
>>>> diff --git a/hw/eeprom93xx.c b/hw/eeprom93xx.c
>>>> index cfa695d..f1d75ec 100644
>>>> --- a/hw/eeprom93xx.c
>>>> +++ b/hw/eeprom93xx.c
>>>> @@ -114,7 +114,7 @@ static const VMStateInfo vmstate_hack_uint16_from_uint8 = {
>>>>    };
>>>>
>>>>    #define VMSTATE_UINT16_HACK_TEST(_f, _s, _t)                           \
>>>> -    VMSTATE_SINGLE_TEST(_f, _s, _t, 0, vmstate_hack_uint16_from_uint8, uint16_t)
>>>> +    VMSTATE_SINGLE_TEST_HACK(_f, _s, _t, 0, vmstate_hack_uint16_from_uint8, uint16_t)
>>>>
>>>>    static bool is_old_eeprom_version(void *opaque, int version_id)
>>>>    {
>> Could we get away with just doing:
>>
>> VMSTATE_UNUSED(3),
>> VMSTATE_UINT8(bar, ...),
> Remember that we are "supposed to be" big/little endian safe.

We always send in network byte order (big endian) so this is safe.

>> That's fully compatible on the wire and seems to be a clearer
>> expression of exactly what the problem is.
> if we are going to break big endian machines, I fully agree.

The migration protocol is always big endian, see:

void qemu_put_be32(QEMUFile *f, unsigned int v)
{
     qemu_put_byte(f, v >> 24);
     qemu_put_byte(f, v >> 16);
     qemu_put_byte(f, v >> 8);
     qemu_put_byte(f, v);
}

So this is completely safe.

Regards,

ANthony Liguori

> Later, Juan.

Patch

diff --git a/hw/eeprom93xx.c b/hw/eeprom93xx.c
index cfa695d..f1d75ec 100644
--- a/hw/eeprom93xx.c
+++ b/hw/eeprom93xx.c
@@ -114,7 +114,7 @@  static const VMStateInfo vmstate_hack_uint16_from_uint8 = {
 };
 
 #define VMSTATE_UINT16_HACK_TEST(_f, _s, _t)                           \
-    VMSTATE_SINGLE_TEST(_f, _s, _t, 0, vmstate_hack_uint16_from_uint8, uint16_t)
+    VMSTATE_SINGLE_TEST_HACK(_f, _s, _t, 0, vmstate_hack_uint16_from_uint8, uint16_t)
 
 static bool is_old_eeprom_version(void *opaque, int version_id)
 {