| Message ID | 20210613102614.5438-1-mark.cave-ayland@ilande.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | esp: fix migration version check in esp_is_version_5() | expand |
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) >
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.
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
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. >
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.
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.
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.
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)
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(-)