| Message ID | 20161018105724.26520-3-pasic@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
* Halil Pasic (pasic@linux.vnet.ibm.com) wrote: > The handling of VMStateField.start is currently quite broken if > VMS_ALLOC is present (that is for VMSTATE_VBUFFER_ALLOC_UINT32) but > fortunately also quite underutilized -- nobody is using .start != 0. > > Let's prove with this patch that it's really broken (as a first > step towards fixing things up). You can't play this trick - patch series have to work and pass tests after each test, otherwise it messes up people in the future trying to do a git bisect. Dave > Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com> > Reviewed-by: Guenther Hutzl <hutzl@linux.vnet.ibm.com> > --- > > The idea is to remove .start support and this patch should > be reverted, as soon this happens, or even better just > dropped. If however dropping the support for .start encounters > resistance, this patch should prove useful in an unexpected > way. > --- > tests/test-vmstate.c | 49 ++++++++++++++++++++++++++++++++++++++++++++++++- > 1 file changed, 48 insertions(+), 1 deletion(-) > > diff --git a/tests/test-vmstate.c b/tests/test-vmstate.c > index 9a57aa0..a2ef4a8 100644 > --- a/tests/test-vmstate.c > +++ b/tests/test-vmstate.c > @@ -588,6 +588,53 @@ static void test_complex_vbuffer(void) > #undef FIELD_EQUAL > #undef BUFFER_EQUAL > > +typedef struct { > + uint32_t vbuff_size; > + uint8_t *vbuff; > + uint64_t stuff; > +} TestVBufStart; > + > +static const VMStateDescription vmstate_vbuff_alloc_start = { > + .name = "test/vbuff_alloc_start", > + .version_id = 1, > + .minimum_version_id = 1, > + .fields = (VMStateField[]) { > + VMSTATE_UINT64(stuff, TestVBufStart), > + VMSTATE_UINT32(vbuff_size, TestVBufStart), > + VMSTATE_VBUFFER_ALLOC_UINT32(vbuff, TestVBufStart, 1, 0, 1, vbuff_size), > + VMSTATE_END_OF_LIST() > + } > + > +}; > + > +static void load_vmstate_one_obj(const VMStateDescription *vmsd, void *obj, > + int version_id) > +{ > + QEMUFile *fload = open_test_file(false); > + > + SUCCESS(vmstate_load_state(fload, vmsd, obj, version_id)); > + qemu_fclose(fload); > +} > + > +static void test_vbuff_alloc_start(void) > +{ > + uint8_t my_vbuff1[] = {0, 1, 2, 3}; > + uint8_t my_vbuff2[] = {0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff}; > + TestVBufStart sample = { > + .stuff = 1, > + .vbuff_size = 3, > + .vbuff = my_vbuff1, > + }; > + TestVBufStart load_obj = { > + .vbuff = my_vbuff2, > + }; > + > + save_vmstate(&vmstate_vbuff_alloc_start, &sample); > + load_vmstate_one_obj(&vmstate_vbuff_alloc_start, &load_obj, 1); > + g_assert_cmpint(load_obj.stuff, ==, 0); > + g_assert_cmpint((uint64_t) load_obj.vbuff, !=, (uint64_t) my_vbuff2); > +} > + > int main(int argc, char **argv) > { > temp_fd = mkstemp(temp_file); > @@ -603,8 +650,8 @@ int main(int argc, char **argv) > g_test_add_func("/vmstate/field_exists/save/noskip", test_save_noskip); > g_test_add_func("/vmstate/field_exists/save/skip", test_save_skip); > g_test_add_func("/vmstate/complex/vbuffer", test_complex_vbuffer); > + g_test_add_func("/vmstate/vbuff/alloc_start", test_vbuff_alloc_start); > g_test_run(); > - > close(temp_fd); > unlink(temp_file); > > -- > 2.8.4 > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On 10/18/2016 03:27 PM, Dr. David Alan Gilbert wrote: > * Halil Pasic (pasic@linux.vnet.ibm.com) wrote: >> > The handling of VMStateField.start is currently quite broken if >> > VMS_ALLOC is present (that is for VMSTATE_VBUFFER_ALLOC_UINT32) but >> > fortunately also quite underutilized -- nobody is using .start != 0. >> > >> > Let's prove with this patch that it's really broken (as a first >> > step towards fixing things up). > You can't play this trick - patch series have to work and pass > tests after each test, otherwise it messes up people in the future > trying to do a git bisect. > > Dave > I think I understand the motivation. Does that mean you are not supposed to expose a bug via a test? I might be able to demonstrate that something is wrong but unable to fix the problem myself (time constraints). How was I supposed to do this? Cheers, Halil
* Halil Pasic (pasic@linux.vnet.ibm.com) wrote: > > > On 10/18/2016 03:27 PM, Dr. David Alan Gilbert wrote: > > * Halil Pasic (pasic@linux.vnet.ibm.com) wrote: > >> > The handling of VMStateField.start is currently quite broken if > >> > VMS_ALLOC is present (that is for VMSTATE_VBUFFER_ALLOC_UINT32) but > >> > fortunately also quite underutilized -- nobody is using .start != 0. > >> > > >> > Let's prove with this patch that it's really broken (as a first > >> > step towards fixing things up). > > You can't play this trick - patch series have to work and pass > > tests after each test, otherwise it messes up people in the future > > trying to do a git bisect. > > > > Dave > > > > I think I understand the motivation. Does that mean > you are not supposed to expose a bug via a test? I might > be able to demonstrate that something is wrong but unable > to fix the problem myself (time constraints). > > How was I supposed to do this? You might add a test but leave it commented out, or just post the test but not for merging so that it only gets merged after someone fixes the bug. Dave > > Cheers, > Halil > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On 10/18/2016 03:54 PM, Dr. David Alan Gilbert wrote: >> > I think I understand the motivation. Does that mean >> > you are not supposed to expose a bug via a test? I might >> > be able to demonstrate that something is wrong but unable >> > to fix the problem myself (time constraints). >> > >> > How was I supposed to do this? > You might add a test but leave it commented out, or just post > the test but not for merging so that it only gets merged > after someone fixes the bug. > > Dave > As stated by the accompanying message: "The idea is to remove .start support and this patch should be reverted, as soon this happens, or even better just dropped. If however dropping the support for .start encounters resistance, this patch should prove useful in an unexpected way." the patch is not intended for a merge. My preferred way of dealing with this is to just pick (merge) the first and the last patch of the series. The second patch is just to prove that we have a problem, and it's effect is immediately reverted by the third patch as a preparation for the forth one which removes the tested feature altogether. In my opinion the inclusion of a commented out test makes even less sense if the tested feature is intended to be removed by the next patch in the series. I think I was not clear enough when stating that this patch is not intended for merging. Is there an established way to do this? Cheers, Halil
* Halil Pasic (pasic@linux.vnet.ibm.com) wrote: > > > On 10/18/2016 03:54 PM, Dr. David Alan Gilbert wrote: > >> > I think I understand the motivation. Does that mean > >> > you are not supposed to expose a bug via a test? I might > >> > be able to demonstrate that something is wrong but unable > >> > to fix the problem myself (time constraints). > >> > > >> > How was I supposed to do this? > > You might add a test but leave it commented out, or just post > > the test but not for merging so that it only gets merged > > after someone fixes the bug. > > > > Dave > > > > As stated by the accompanying message: > > "The idea is to remove .start support and this patch should > be reverted, as soon this happens, or even better just > dropped. If however dropping the support for .start encounters > resistance, this patch should prove useful in an unexpected > way." > > the patch is not intended for a merge. My preferred way of dealing > with this is to just pick (merge) the first and the last patch of the > series. The second patch is just to prove that we have a problem, > and it's effect is immediately reverted by the third patch as a > preparation for the forth one which removes the tested feature altogether. > > In my opinion the inclusion of a commented out test makes even less > sense if the tested feature is intended to be removed by the next > patch in the series. > > I think I was not clear enough when stating that this patch is > not intended for merging. Is there an established way to do > this? I don't think there's any point in posting it like that as part of a patch series; posting it as a separate test that fails or something like that; but I don't think I've ever seen it done like that inside a patch series where you expect some of it to be picked up. Dave > > Cheers, > Halil > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On 10/18/2016 08:32 PM, Dr. David Alan Gilbert wrote: >> > "The idea is to remove .start support and this patch should >> > be reverted, as soon this happens, or even better just >> > dropped. If however dropping the support for .start encounters >> > resistance, this patch should prove useful in an unexpected >> > way." >> > >> > the patch is not intended for a merge. My preferred way of dealing >> > with this is to just pick (merge) the first and the last patch of the >> > series. The second patch is just to prove that we have a problem, >> > and it's effect is immediately reverted by the third patch as a >> > preparation for the forth one which removes the tested feature altogether. >> > >> > In my opinion the inclusion of a commented out test makes even less >> > sense if the tested feature is intended to be removed by the next >> > patch in the series. >> > >> > I think I was not clear enough when stating that this patch is >> > not intended for merging. Is there an established way to do >> > this? > I don't think there's any point in posting it like that as part > of a patch series; posting it as a separate test that fails or > something like that; but I don't think I've ever seen it done > like that inside a patch series where you expect some of it > to be picked up. > > Dave > I understand. I assumed cherry-picking the two relevant patches from the series would not be a problem here. I was wrong. Next time I will make sure to either do a separate failing test patch and and cross reference in the cover letters, or to first do the fix and then improve the test coverage so the bug does not come back. Should I send a v2 with the two questionable patches (the failing test and the revert of it) removed right away? Regards, Halil
* Halil Pasic (pasic@linux.vnet.ibm.com) wrote: > > > On 10/18/2016 08:32 PM, Dr. David Alan Gilbert wrote: > >> > "The idea is to remove .start support and this patch should > >> > be reverted, as soon this happens, or even better just > >> > dropped. If however dropping the support for .start encounters > >> > resistance, this patch should prove useful in an unexpected > >> > way." > >> > > >> > the patch is not intended for a merge. My preferred way of dealing > >> > with this is to just pick (merge) the first and the last patch of the > >> > series. The second patch is just to prove that we have a problem, > >> > and it's effect is immediately reverted by the third patch as a > >> > preparation for the forth one which removes the tested feature altogether. > >> > > >> > In my opinion the inclusion of a commented out test makes even less > >> > sense if the tested feature is intended to be removed by the next > >> > patch in the series. > >> > > >> > I think I was not clear enough when stating that this patch is > >> > not intended for merging. Is there an established way to do > >> > this? > > I don't think there's any point in posting it like that as part > > of a patch series; posting it as a separate test that fails or > > something like that; but I don't think I've ever seen it done > > like that inside a patch series where you expect some of it > > to be picked up. > > > > Dave > > > > I understand. I assumed cherry-picking the two relevant patches from the > series would not be a problem here. I was wrong. > > Next time I will make sure to either do a separate failing test patch > and and cross reference in the cover letters, or to first do the fix and > then improve the test coverage so the bug does not come back. > > Should I send a v2 with the two questionable patches (the failing test > and the revert of it) removed right away? Yes, probably the best way; you can add the Review-by's I've just sent. Dave > Regards, > Halil > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On 10/20/2016 02:00 PM, Dr. David Alan Gilbert wrote: >> Should I send a v2 with the two questionable patches (the failing test >> > and the revert of it) removed right away? > Yes, probably the best way; you can add the Review-by's I've just > sent. > > Dave > Thanks a lot. Will do! Halil
diff --git a/tests/test-vmstate.c b/tests/test-vmstate.c index 9a57aa0..a2ef4a8 100644 --- a/tests/test-vmstate.c +++ b/tests/test-vmstate.c @@ -588,6 +588,53 @@ static void test_complex_vbuffer(void) #undef FIELD_EQUAL #undef BUFFER_EQUAL +typedef struct { + uint32_t vbuff_size; + uint8_t *vbuff; + uint64_t stuff; +} TestVBufStart; + +static const VMStateDescription vmstate_vbuff_alloc_start = { + .name = "test/vbuff_alloc_start", + .version_id = 1, + .minimum_version_id = 1, + .fields = (VMStateField[]) { + VMSTATE_UINT64(stuff, TestVBufStart), + VMSTATE_UINT32(vbuff_size, TestVBufStart), + VMSTATE_VBUFFER_ALLOC_UINT32(vbuff, TestVBufStart, 1, 0, 1, vbuff_size), + VMSTATE_END_OF_LIST() + } + +}; + +static void load_vmstate_one_obj(const VMStateDescription *vmsd, void *obj, + int version_id) +{ + QEMUFile *fload = open_test_file(false); + + SUCCESS(vmstate_load_state(fload, vmsd, obj, version_id)); + qemu_fclose(fload); +} + +static void test_vbuff_alloc_start(void) +{ + uint8_t my_vbuff1[] = {0, 1, 2, 3}; + uint8_t my_vbuff2[] = {0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff}; + TestVBufStart sample = { + .stuff = 1, + .vbuff_size = 3, + .vbuff = my_vbuff1, + }; + TestVBufStart load_obj = { + .vbuff = my_vbuff2, + }; + + save_vmstate(&vmstate_vbuff_alloc_start, &sample); + load_vmstate_one_obj(&vmstate_vbuff_alloc_start, &load_obj, 1); + g_assert_cmpint(load_obj.stuff, ==, 0); + g_assert_cmpint((uint64_t) load_obj.vbuff, !=, (uint64_t) my_vbuff2); +} + int main(int argc, char **argv) { temp_fd = mkstemp(temp_file); @@ -603,8 +650,8 @@ int main(int argc, char **argv) g_test_add_func("/vmstate/field_exists/save/noskip", test_save_noskip); g_test_add_func("/vmstate/field_exists/save/skip", test_save_skip); g_test_add_func("/vmstate/complex/vbuffer", test_complex_vbuffer); + g_test_add_func("/vmstate/vbuff/alloc_start", test_vbuff_alloc_start); g_test_run(); - close(temp_fd); unlink(temp_file);