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

[Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>]

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.