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.