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 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.