[PATCH] esp: fix migration version check in esp_is_version_5()

[PATCH] esp: fix migration version check in esp_is_version_5()

[Mark Cave-Ayland]

Commit 4e78f3bf35 "esp: defer command completion interrupt on incoming data
transfers" added a version check for use with VMSTATE_*_TEST macros to allow
migration from older QEMU versions. Unfortunately the version check fails to
work in its current form since if the VMStateDescription version_id is
incremented, the test returns false and so the fields are not included in the
outgoing migration stream.

Change the version check to use >= rather == to ensure that migration works
correctly when the ESPState VMStateDescription has version_id > 5.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Fixes: 4e78f3bf35 ("esp: defer command completion interrupt on incoming data transfers")
---
 hw/scsi/esp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index bfdb94292b..39756ddd99 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -1120,7 +1120,7 @@ static bool esp_is_version_5(void *opaque, int version_id)
     ESPState *s = ESP(opaque);
 
     version_id = MIN(version_id, s->mig_version_id);
-    return version_id == 5;
+    return version_id >= 5;
 }
 
 int esp_pre_save(void *opaque)
-- 
2.20.1

Re: [PATCH] esp: fix migration version check in esp_is_version_5()

[Philippe Mathieu-Daudé]

On 6/13/21 12:26 PM, Mark Cave-Ayland wrote:
> Commit 4e78f3bf35 "esp: defer command completion interrupt on incoming data
> transfers" added a version check for use with VMSTATE_*_TEST macros to allow
> migration from older QEMU versions. Unfortunately the version check fails to
> work in its current form since if the VMStateDescription version_id is
> incremented, the test returns false and so the fields are not included in the
> outgoing migration stream.
> 
> Change the version check to use >= rather == to ensure that migration works
> correctly when the ESPState VMStateDescription has version_id > 5.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> Fixes: 4e78f3bf35 ("esp: defer command completion interrupt on incoming data transfers")
> ---
Well, it is not buggy yet :)

>  hw/scsi/esp.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
> index bfdb94292b..39756ddd99 100644
> --- a/hw/scsi/esp.c
> +++ b/hw/scsi/esp.c
> @@ -1120,7 +1120,7 @@ static bool esp_is_version_5(void *opaque, int version_id)

Can you rename esp_is_at_least_version_5()?

>      ESPState *s = ESP(opaque);
>  
>      version_id = MIN(version_id, s->mig_version_id);
> -    return version_id == 5;
> +    return version_id >= 5;
>  }
>  
>  int esp_pre_save(void *opaque)
> 

Re: [PATCH] esp: fix migration version check in esp_is_version_5()

[Mark Cave-Ayland]

On 14/06/2021 06:42, Philippe Mathieu-Daudé wrote:

> On 6/13/21 12:26 PM, Mark Cave-Ayland wrote:
>> Commit 4e78f3bf35 "esp: defer command completion interrupt on incoming data
>> transfers" added a version check for use with VMSTATE_*_TEST macros to allow
>> migration from older QEMU versions. Unfortunately the version check fails to
>> work in its current form since if the VMStateDescription version_id is
>> incremented, the test returns false and so the fields are not included in the
>> outgoing migration stream.
>>
>> Change the version check to use >= rather == to ensure that migration works
>> correctly when the ESPState VMStateDescription has version_id > 5.
>>
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> Fixes: 4e78f3bf35 ("esp: defer command completion interrupt on incoming data transfers")
>> ---
> Well, it is not buggy yet :)

:)

>>   hw/scsi/esp.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
>> index bfdb94292b..39756ddd99 100644
>> --- a/hw/scsi/esp.c
>> +++ b/hw/scsi/esp.c
>> @@ -1120,7 +1120,7 @@ static bool esp_is_version_5(void *opaque, int version_id)
> 
> Can you rename esp_is_at_least_version_5()?

Sure, I can rename it if you like but it will of course make the diff noisier. 
esp_is_at_least_version_5() seems quite a mouthful though, what about 
esp_min_version_5() instead?

>>       ESPState *s = ESP(opaque);
>>   
>>       version_id = MIN(version_id, s->mig_version_id);
>> -    return version_id == 5;
>> +    return version_id >= 5;
>>   }
>>   
>>   int esp_pre_save(void *opaque)


ATB,

Mark.

Re: [PATCH] esp: fix migration version check in esp_is_version_5()

[Paolo Bonzini]

On 13/06/21 12:26, Mark Cave-Ayland wrote:
> Commit 4e78f3bf35 "esp: defer command completion interrupt on incoming data
> transfers" added a version check for use with VMSTATE_*_TEST macros to allow
> migration from older QEMU versions. Unfortunately the version check fails to
> work in its current form since if the VMStateDescription version_id is
> incremented, the test returns false and so the fields are not included in the
> outgoing migration stream.
> 
> Change the version check to use >= rather == to ensure that migration works
> correctly when the ESPState VMStateDescription has version_id > 5.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> Fixes: 4e78f3bf35 ("esp: defer command completion interrupt on incoming data transfers")
> ---
>   hw/scsi/esp.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
> index bfdb94292b..39756ddd99 100644
> --- a/hw/scsi/esp.c
> +++ b/hw/scsi/esp.c
> @@ -1120,7 +1120,7 @@ static bool esp_is_version_5(void *opaque, int version_id)
>       ESPState *s = ESP(opaque);
>   
>       version_id = MIN(version_id, s->mig_version_id);
> -    return version_id == 5;
> +    return version_id >= 5;
>   }
>   
>   int esp_pre_save(void *opaque)
> 

You can use the _V version of the macros and get rid of this function 
altogether.  VMSTATE_FIFO8_TEST is not used outside esp.c so it can also 
be replaced with VMSTATE_FIFO8_V, just initializing .version_id in place 
of .field_exists.

Paolo

Re: [PATCH] esp: fix migration version check in esp_is_version_5()

[Philippe Mathieu-Daudé]

On 6/14/21 9:44 AM, Mark Cave-Ayland wrote:
> On 14/06/2021 06:42, Philippe Mathieu-Daudé wrote:
> 
>> On 6/13/21 12:26 PM, Mark Cave-Ayland wrote:
>>> Commit 4e78f3bf35 "esp: defer command completion interrupt on
>>> incoming data
>>> transfers" added a version check for use with VMSTATE_*_TEST macros
>>> to allow
>>> migration from older QEMU versions. Unfortunately the version check
>>> fails to
>>> work in its current form since if the VMStateDescription version_id is
>>> incremented, the test returns false and so the fields are not
>>> included in the
>>> outgoing migration stream.
>>>
>>> Change the version check to use >= rather == to ensure that migration
>>> works
>>> correctly when the ESPState VMStateDescription has version_id > 5.
>>>
>>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>>> Fixes: 4e78f3bf35 ("esp: defer command completion interrupt on
>>> incoming data transfers")
>>> ---
>> Well, it is not buggy yet :)
> 
> :)
> 
>>>   hw/scsi/esp.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
>>> index bfdb94292b..39756ddd99 100644
>>> --- a/hw/scsi/esp.c
>>> +++ b/hw/scsi/esp.c
>>> @@ -1120,7 +1120,7 @@ static bool esp_is_version_5(void *opaque, int
>>> version_id)
>>
>> Can you rename esp_is_at_least_version_5()?
> 
> Sure, I can rename it if you like but it will of course make the diff
> noisier. esp_is_at_least_version_5() seems quite a mouthful though, what
> about esp_min_version_5() instead?

I was looking at esp_is_before_version_5(). Following that logic it
should be named esp_is_after_version_4()? Or esp_min_version_5() and
rename esp_is_before_version_5() -> esp_max_version_4(). All options
seem confuse...

Maybe _V macros suggested by Paolo make all clearer?

> 
>>>       ESPState *s = ESP(opaque);
>>>         version_id = MIN(version_id, s->mig_version_id);
>>> -    return version_id == 5;
>>> +    return version_id >= 5;
>>>   }
>>>     int esp_pre_save(void *opaque)
> 
> 
> ATB,
> 
> Mark.
> 

Re: [PATCH] esp: fix migration version check in esp_is_version_5()

[Mark Cave-Ayland]

On 14/06/2021 10:01, Philippe Mathieu-Daudé wrote:

> On 6/14/21 9:44 AM, Mark Cave-Ayland wrote:
>> On 14/06/2021 06:42, Philippe Mathieu-Daudé wrote:
>>
>>> On 6/13/21 12:26 PM, Mark Cave-Ayland wrote:
>>>> Commit 4e78f3bf35 "esp: defer command completion interrupt on
>>>> incoming data
>>>> transfers" added a version check for use with VMSTATE_*_TEST macros
>>>> to allow
>>>> migration from older QEMU versions. Unfortunately the version check
>>>> fails to
>>>> work in its current form since if the VMStateDescription version_id is
>>>> incremented, the test returns false and so the fields are not
>>>> included in the
>>>> outgoing migration stream.
>>>>
>>>> Change the version check to use >= rather == to ensure that migration
>>>> works
>>>> correctly when the ESPState VMStateDescription has version_id > 5.
>>>>
>>>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>>>> Fixes: 4e78f3bf35 ("esp: defer command completion interrupt on
>>>> incoming data transfers")
>>>> ---
>>> Well, it is not buggy yet :)
>>
>> :)
>>
>>>>    hw/scsi/esp.c | 2 +-
>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
>>>> index bfdb94292b..39756ddd99 100644
>>>> --- a/hw/scsi/esp.c
>>>> +++ b/hw/scsi/esp.c
>>>> @@ -1120,7 +1120,7 @@ static bool esp_is_version_5(void *opaque, int
>>>> version_id)
>>>
>>> Can you rename esp_is_at_least_version_5()?
>>
>> Sure, I can rename it if you like but it will of course make the diff
>> noisier. esp_is_at_least_version_5() seems quite a mouthful though, what
>> about esp_min_version_5() instead?
> 
> I was looking at esp_is_before_version_5(). Following that logic it
> should be named esp_is_after_version_4()? Or esp_min_version_5() and
> rename esp_is_before_version_5() -> esp_max_version_4(). All options
> seem confuse...
> 
> Maybe _V macros suggested by Paolo make all clearer?

Unfortunately the _V macros don't work correctly here (see my previous reply to 
Paolo) which is why these functions exist in the first place.

If all the proposed options seem equally confusing, is it worth just sticking with 
what was in the original patch? Otherwise we end up with a whole series renaming 
functions in a way we're still not happy with, compared with the original patch which 
is effectively a diff of 1 character.


ATB,

Mark.

Re: [PATCH] esp: fix migration version check in esp_is_version_5()

[Philippe Mathieu-Daudé]

On 6/14/21 1:59 PM, Mark Cave-Ayland wrote:
> On 14/06/2021 10:01, Philippe Mathieu-Daudé wrote:
> 
>> On 6/14/21 9:44 AM, Mark Cave-Ayland wrote:
>>> On 14/06/2021 06:42, Philippe Mathieu-Daudé wrote:
>>>
>>>> On 6/13/21 12:26 PM, Mark Cave-Ayland wrote:
>>>>> Commit 4e78f3bf35 "esp: defer command completion interrupt on
>>>>> incoming data
>>>>> transfers" added a version check for use with VMSTATE_*_TEST macros
>>>>> to allow
>>>>> migration from older QEMU versions. Unfortunately the version check
>>>>> fails to
>>>>> work in its current form since if the VMStateDescription version_id is
>>>>> incremented, the test returns false and so the fields are not
>>>>> included in the
>>>>> outgoing migration stream.
>>>>>
>>>>> Change the version check to use >= rather == to ensure that migration
>>>>> works
>>>>> correctly when the ESPState VMStateDescription has version_id > 5.
>>>>>
>>>>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>>>>> Fixes: 4e78f3bf35 ("esp: defer command completion interrupt on
>>>>> incoming data transfers")
>>>>> ---
>>>> Well, it is not buggy yet :)
>>>
>>> :)
>>>
>>>>>    hw/scsi/esp.c | 2 +-
>>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
>>>>> index bfdb94292b..39756ddd99 100644
>>>>> --- a/hw/scsi/esp.c
>>>>> +++ b/hw/scsi/esp.c
>>>>> @@ -1120,7 +1120,7 @@ static bool esp_is_version_5(void *opaque, int
>>>>> version_id)
>>>>
>>>> Can you rename esp_is_at_least_version_5()?
>>>
>>> Sure, I can rename it if you like but it will of course make the diff
>>> noisier. esp_is_at_least_version_5() seems quite a mouthful though, what
>>> about esp_min_version_5() instead?
>>
>> I was looking at esp_is_before_version_5(). Following that logic it
>> should be named esp_is_after_version_4()? Or esp_min_version_5() and
>> rename esp_is_before_version_5() -> esp_max_version_4(). All options
>> seem confuse...
>>
>> Maybe _V macros suggested by Paolo make all clearer?
> 
> Unfortunately the _V macros don't work correctly here (see my previous
> reply to Paolo) which is why these functions exist in the first place.
> 
> If all the proposed options seem equally confusing, is it worth just
> sticking with what was in the original patch? Otherwise we end up with a
> whole series renaming functions in a way we're still not happy with,
> compared with the original patch which is effectively a diff of 1
> character.

Fine, you are likely the next one going to modify these functions,
so I don't mind.

Re: [PATCH] esp: fix migration version check in esp_is_version_5()

[Mark Cave-Ayland]

On 14/06/2021 14:47, Philippe Mathieu-Daudé wrote:

> On 6/14/21 1:59 PM, Mark Cave-Ayland wrote:
>> On 14/06/2021 10:01, Philippe Mathieu-Daudé wrote:
>>
>>> On 6/14/21 9:44 AM, Mark Cave-Ayland wrote:
>>>> On 14/06/2021 06:42, Philippe Mathieu-Daudé wrote:
>>>>
>>>>> On 6/13/21 12:26 PM, Mark Cave-Ayland wrote:
>>>>>> Commit 4e78f3bf35 "esp: defer command completion interrupt on
>>>>>> incoming data
>>>>>> transfers" added a version check for use with VMSTATE_*_TEST macros
>>>>>> to allow
>>>>>> migration from older QEMU versions. Unfortunately the version check
>>>>>> fails to
>>>>>> work in its current form since if the VMStateDescription version_id is
>>>>>> incremented, the test returns false and so the fields are not
>>>>>> included in the
>>>>>> outgoing migration stream.
>>>>>>
>>>>>> Change the version check to use >= rather == to ensure that migration
>>>>>> works
>>>>>> correctly when the ESPState VMStateDescription has version_id > 5.
>>>>>>
>>>>>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>>>>>> Fixes: 4e78f3bf35 ("esp: defer command completion interrupt on
>>>>>> incoming data transfers")
>>>>>> ---
>>>>> Well, it is not buggy yet :)
>>>>
>>>> :)
>>>>
>>>>>>     hw/scsi/esp.c | 2 +-
>>>>>>     1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
>>>>>> index bfdb94292b..39756ddd99 100644
>>>>>> --- a/hw/scsi/esp.c
>>>>>> +++ b/hw/scsi/esp.c
>>>>>> @@ -1120,7 +1120,7 @@ static bool esp_is_version_5(void *opaque, int
>>>>>> version_id)
>>>>>
>>>>> Can you rename esp_is_at_least_version_5()?
>>>>
>>>> Sure, I can rename it if you like but it will of course make the diff
>>>> noisier. esp_is_at_least_version_5() seems quite a mouthful though, what
>>>> about esp_min_version_5() instead?
>>>
>>> I was looking at esp_is_before_version_5(). Following that logic it
>>> should be named esp_is_after_version_4()? Or esp_min_version_5() and
>>> rename esp_is_before_version_5() -> esp_max_version_4(). All options
>>> seem confuse...
>>>
>>> Maybe _V macros suggested by Paolo make all clearer?
>>
>> Unfortunately the _V macros don't work correctly here (see my previous
>> reply to Paolo) which is why these functions exist in the first place.
>>
>> If all the proposed options seem equally confusing, is it worth just
>> sticking with what was in the original patch? Otherwise we end up with a
>> whole series renaming functions in a way we're still not happy with,
>> compared with the original patch which is effectively a diff of 1
>> character.
> 
> Fine, you are likely the next one going to modify these functions,
> so I don't mind.

Thanks. I had another think about this over the yesterday evening to see if I could 
come up with anything better, but didn't manage to find any ideas that were an 
improvement in all areas. So let's stick with this for now.

Paolo - I'm happy for you to queue this along with the other ESP patches.


ATB,

Mark.