| Message ID | 20170606165510.33057-4-pasic@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
* Halil Pasic (pasic@linux.vnet.ibm.com) wrote: > This one has to be fixed up to 's390x: vmstatify config migration for > virtio-ccw' provided we want to achieve the same as 's390x/css: catch > section mismatch on load' does. > > Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com> > --- > > This is on tom of 's390x: vmstatify config migration for virtio-ccw' p > which ain't on top of 's390x/css: catch section mismatch on load' but on > top of master. I kind of have a circular dependency here. This is why > the series is RFC. > > Wanted to provide an usage example. Faked 'Re: ' so patchew does not > try to apply this on top of current master. > --- > hw/s390x/css.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/hw/s390x/css.c b/hw/s390x/css.c > index 348129e1b2..de277d6a3d 100644 > --- a/hw/s390x/css.c > +++ b/hw/s390x/css.c > @@ -134,6 +134,10 @@ static const VMStateDescription vmstate_sense_id = { > static int subch_dev_post_load(void *opaque, int version_id); > static void subch_dev_pre_save(void *opaque); > > +const char err_hint_devno[] = "Devno mismatch, tried to load wrong section!" > + " Likely reason: some sequences of plug and unplug can break" > + " migration for machine versions prior to 2.7 (known design flaw)."; > + That's ok, but I suggest: * 'bug' rather than 'design flaw' - it sounds a bit less scary to endusers. Other than that, Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com> Dave > const VMStateDescription vmstate_subch_dev = { > .name = "s390_subch_dev", > .version_id = 1, > @@ -144,7 +148,7 @@ const VMStateDescription vmstate_subch_dev = { > VMSTATE_UINT8_EQUAL(cssid, SubchDev), > VMSTATE_UINT8_EQUAL(ssid, SubchDev), > VMSTATE_UINT16(migrated_schid, SubchDev), > - VMSTATE_UINT16(devno, SubchDev), > + VMSTATE_UINT16_EQUAL_HINT(devno, SubchDev, err_hint_devno), > VMSTATE_BOOL(thinint_active, SubchDev), > VMSTATE_STRUCT(curr_status, SubchDev, 0, vmstate_schib, SCHIB), > VMSTATE_UINT8_ARRAY(sense_data, SubchDev, 32), > -- > 2.11.2 > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On 06/07/2017 01:22 PM, Dr. David Alan Gilbert wrote: > * Halil Pasic (pasic@linux.vnet.ibm.com) wrote: >> This one has to be fixed up to 's390x: vmstatify config migration for >> virtio-ccw' provided we want to achieve the same as 's390x/css: catch >> section mismatch on load' does. >> >> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com> >> --- >> >> This is on tom of 's390x: vmstatify config migration for virtio-ccw' > p >> which ain't on top of 's390x/css: catch section mismatch on load' but on >> top of master. I kind of have a circular dependency here. This is why >> the series is RFC. >> >> Wanted to provide an usage example. Faked 'Re: ' so patchew does not >> try to apply this on top of current master. >> --- >> hw/s390x/css.c | 6 +++++- >> 1 file changed, 5 insertions(+), 1 deletion(-) >> >> diff --git a/hw/s390x/css.c b/hw/s390x/css.c >> index 348129e1b2..de277d6a3d 100644 >> --- a/hw/s390x/css.c >> +++ b/hw/s390x/css.c >> @@ -134,6 +134,10 @@ static const VMStateDescription vmstate_sense_id = { >> static int subch_dev_post_load(void *opaque, int version_id); >> static void subch_dev_pre_save(void *opaque); >> >> +const char err_hint_devno[] = "Devno mismatch, tried to load wrong section!" >> + " Likely reason: some sequences of plug and unplug can break" >> + " migration for machine versions prior to 2.7 (known design flaw)."; >> + > > That's ok, but I suggest: > * 'bug' rather than 'design flaw' - it sounds a bit less scary to > endusers. > I do not think it can be changed now. Christian as already sent out a pull request for the patch this series is re-doing with vmstate. That patch has the same error message. I have considered bug but decided against because the 'bug' can't be fixed. I think it's pretty hard to hit this with normal usage (and that's probably the reason why it went undetected until recently), so hope not too may endusers are going to get scared by developer honesty :). > Other than that, > > > Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com> Great thanks for the review! Halil
* Halil Pasic (pasic@linux.vnet.ibm.com) wrote: > > > On 06/07/2017 01:22 PM, Dr. David Alan Gilbert wrote: > > * Halil Pasic (pasic@linux.vnet.ibm.com) wrote: > >> This one has to be fixed up to 's390x: vmstatify config migration for > >> virtio-ccw' provided we want to achieve the same as 's390x/css: catch > >> section mismatch on load' does. > >> > >> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com> > >> --- > >> > >> This is on tom of 's390x: vmstatify config migration for virtio-ccw' > > p > >> which ain't on top of 's390x/css: catch section mismatch on load' but on > >> top of master. I kind of have a circular dependency here. This is why > >> the series is RFC. > >> > >> Wanted to provide an usage example. Faked 'Re: ' so patchew does not > >> try to apply this on top of current master. > >> --- > >> hw/s390x/css.c | 6 +++++- > >> 1 file changed, 5 insertions(+), 1 deletion(-) > >> > >> diff --git a/hw/s390x/css.c b/hw/s390x/css.c > >> index 348129e1b2..de277d6a3d 100644 > >> --- a/hw/s390x/css.c > >> +++ b/hw/s390x/css.c > >> @@ -134,6 +134,10 @@ static const VMStateDescription vmstate_sense_id = { > >> static int subch_dev_post_load(void *opaque, int version_id); > >> static void subch_dev_pre_save(void *opaque); > >> > >> +const char err_hint_devno[] = "Devno mismatch, tried to load wrong section!" > >> + " Likely reason: some sequences of plug and unplug can break" > >> + " migration for machine versions prior to 2.7 (known design flaw)."; > >> + > > > > That's ok, but I suggest: > > * 'bug' rather than 'design flaw' - it sounds a bit less scary to > > endusers. > > > > I do not think it can be changed now. Christian as already sent out a pull > request for the patch this series is re-doing with vmstate. That patch has the > same error message. I have considered bug but decided against because > the 'bug' can't be fixed. I think it's pretty hard to hit this with normal > usage (and that's probably the reason why it went undetected until recently), > so hope not too may endusers are going to get scared by developer honesty :). No problem; it's your device anyway :-) > > Other than that, > > > > > > Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > > Great thanks for the review! Dave > Halil > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
"Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote: > * Halil Pasic (pasic@linux.vnet.ibm.com) wrote: >> This one has to be fixed up to 's390x: vmstatify config migration for >> virtio-ccw' provided we want to achieve the same as 's390x/css: catch >> section mismatch on load' does. >> >> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com> >> --- >> >> This is on tom of 's390x: vmstatify config migration for virtio-ccw' > p >> which ain't on top of 's390x/css: catch section mismatch on load' but on >> top of master. I kind of have a circular dependency here. This is why >> the series is RFC. >> >> Wanted to provide an usage example. Faked 'Re: ' so patchew does not >> try to apply this on top of current master. >> --- >> hw/s390x/css.c | 6 +++++- >> 1 file changed, 5 insertions(+), 1 deletion(-) >> >> diff --git a/hw/s390x/css.c b/hw/s390x/css.c >> index 348129e1b2..de277d6a3d 100644 >> --- a/hw/s390x/css.c >> +++ b/hw/s390x/css.c >> @@ -134,6 +134,10 @@ static const VMStateDescription vmstate_sense_id = { >> static int subch_dev_post_load(void *opaque, int version_id); >> static void subch_dev_pre_save(void *opaque); >> >> +const char err_hint_devno[] = "Devno mismatch, tried to load wrong section!" >> + " Likely reason: some sequences of plug and unplug can break" >> + " migration for machine versions prior to 2.7 (known design flaw)."; >> + > > That's ok, but I suggest: > * 'bug' rather than 'design flaw' - it sounds a bit less scary to > endusers. If we are being politically correct: (known limitation)? O:-) Reviewed-by: Juan Quintela <quintela@redhat.com> Later, Juan.
diff --git a/hw/s390x/css.c b/hw/s390x/css.c index 348129e1b2..de277d6a3d 100644 --- a/hw/s390x/css.c +++ b/hw/s390x/css.c @@ -134,6 +134,10 @@ static const VMStateDescription vmstate_sense_id = { static int subch_dev_post_load(void *opaque, int version_id); static void subch_dev_pre_save(void *opaque); +const char err_hint_devno[] = "Devno mismatch, tried to load wrong section!" + " Likely reason: some sequences of plug and unplug can break" + " migration for machine versions prior to 2.7 (known design flaw)."; + const VMStateDescription vmstate_subch_dev = { .name = "s390_subch_dev", .version_id = 1, @@ -144,7 +148,7 @@ const VMStateDescription vmstate_subch_dev = { VMSTATE_UINT8_EQUAL(cssid, SubchDev), VMSTATE_UINT8_EQUAL(ssid, SubchDev), VMSTATE_UINT16(migrated_schid, SubchDev), - VMSTATE_UINT16(devno, SubchDev), + VMSTATE_UINT16_EQUAL_HINT(devno, SubchDev, err_hint_devno), VMSTATE_BOOL(thinint_active, SubchDev), VMSTATE_STRUCT(curr_status, SubchDev, 0, vmstate_schib, SCHIB), VMSTATE_UINT8_ARRAY(sense_data, SubchDev, 32),
This one has to be fixed up to 's390x: vmstatify config migration for virtio-ccw' provided we want to achieve the same as 's390x/css: catch section mismatch on load' does. Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com> --- This is on tom of 's390x: vmstatify config migration for virtio-ccw' which ain't on top of 's390x/css: catch section mismatch on load' but on top of master. I kind of have a circular dependency here. This is why the series is RFC. Wanted to provide an usage example. Faked 'Re: ' so patchew does not try to apply this on top of current master. --- hw/s390x/css.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)