[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2 000/124] VMState Simplification (Massive)
From: |
Juan Quintela |
Subject: |
Re: [Qemu-devel] [PATCH v2 000/124] VMState Simplification (Massive) |
Date: |
Mon, 21 Apr 2014 19:25:30 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.3 (gnu/linux) |
Peter Maydell <address@hidden> wrote:
> On 21 April 2014 17:31, Juan Quintela <address@hidden> wrote:
>> Patches are easy to review in sequence, any of them is very simple, and
>> the few ones that are long (minimum_version_id_old) review is just
>> looking that the previous line is minimum_version_id = <same number>.
>
> But there are simply _far too many_ of them in this series.
> For instance "remove minimum_version_id_old" should be a series
> by itself: it is self contained and can be reviewed and committed
> much more easily that way. It seems likely that there are three or
> four other things also going on in this set of patches.
>
> "Improve the vmstate tests" should also be a separate patchset.
I can split the series at any point (they make sense even without the
rest).
What about:
1,7,8: bug fixes/simplification
2-6: massive Unneeded version_minimum_id_old removal, but trivial per se
9-47: New testing framework and tests for integers and arrays of
integers
This three should be no controversial.
48-86: Removal of _V variants and ends with removal of version_id for
fields
This shouldn't be controversial at all, but I agree that I can be
biased here.
87-124: buffers, pointers and structs, in weird combinations.
This shouldn't be controversial either, but depends on the
removal of version_id. Why? Because otherwise we should need
(roughly) to double the number of tests, as we have the testing
mechanism that is a super set of the version stuff.
Historical remark
-----------------
At the beginning, the testing stuff was not going to be integrated,
because it makes the migration stream non-deterministic. But then, the
reality won :-( That is the reason that we have two mechanisms. Version
stuff should be enough, but it wasn't. So we needed to add the testing
stuff. As you can see for this series, there was a reason why I didn't
changed all users at the time (and the naïve idea that I could remove
the ->field_exist() later).
> I'm not sure about merging the field versioning with the field test
> function stuff, but I'm not about to try to fish the relevant patches
> out of this enormous mess so I'll wait until they appear in a series
> of their own before I comment on them.
Well, at least I now told you were the patches are O:-)
If there is a reason why you don't want to remove it (or anybody else),
please let me know.
The reason for the removal is that we are talking continuously about
changing the migration stream, changing formats, changing everything.
But without a simplification first, things become very, very complicated
fast.
And the other thing that prompted this was the VMSTATE_VALIDATE() from
mst. Basically we are adding another kludge, when we should fix it, in
a more generic way (next series on top of this ones should address it).
> You mention at least one bugfix of some kind: that ought to be
> its own patch or patchset.
> Even 40 patches or so in a set is pushing the boundaries of
> what is reasonably reviewable in my opinion.
What do you think of the plan that I proposed?
If you think that reviewing 124 patches is a pain to review, think how
big of a pain is to maintaining them :-(
Thanks a lot for the tips, and trying to help getting this through.
Later, Juan.
- [Qemu-devel] [PATCH 120/124] vmstate: Test for VMSTATE_STRUCT_VARRAY_POINTER_UINT16, (continued)
- [Qemu-devel] [PATCH 120/124] vmstate: Test for VMSTATE_STRUCT_VARRAY_POINTER_UINT16, Juan Quintela, 2014/04/21
- [Qemu-devel] [PATCH 121/124] vmstate: Test for VMSTATE_STRUCT_ARRAY_POINTER_UINT32, Juan Quintela, 2014/04/21
- [Qemu-devel] [PATCH 122/124] vmstate: Test for VMSTATE_STRUCT_VARRAY_POINTER_INT32, Juan Quintela, 2014/04/21
- [Qemu-devel] [PATCH 123/124] vmstate: Test for VMSTATE_ARRAY_OF_POINTER_TO_STRUCT, Juan Quintela, 2014/04/21
- [Qemu-devel] [PATCH 124/124] vmstate: Test for VMSTATE_ARRAY_OF_POINTER, Juan Quintela, 2014/04/21
- [Qemu-devel] [PATCH 114/124] vmstate: Remove unused VMSTATE_STRUCT_POINTER_TEST, Juan Quintela, 2014/04/21
- [Qemu-devel] [PATCH 110/124] vmstate: Test for VMSTATE_VARRAY_INT32, Juan Quintela, 2014/04/21
- Re: [Qemu-devel] [PATCH v2 000/124] VMState Simplification (Massive), Peter Maydell, 2014/04/21
- Re: [Qemu-devel] [PATCH v2 000/124] VMState Simplification (Massive), Juan Quintela, 2014/04/21
- Re: [Qemu-devel] [PATCH v2 000/124] VMState Simplification (Massive), Peter Maydell, 2014/04/21
- Re: [Qemu-devel] [PATCH v2 000/124] VMState Simplification (Massive),
Juan Quintela <=
- Re: [Qemu-devel] [PATCH v2 000/124] VMState Simplification (Massive), Peter Maydell, 2014/04/21
- Re: [Qemu-devel] [PATCH v2 000/124] VMState Simplification (Massive), Paolo Bonzini, 2014/04/21
- Re: [Qemu-devel] [PATCH v2 000/124] VMState Simplification (Massive), Dr. David Alan Gilbert, 2014/04/22
- Re: [Qemu-devel] [PATCH v2 000/124] VMState Simplification (Massive), Paolo Bonzini, 2014/04/27
- Re: [Qemu-devel] [PATCH v2 000/124] VMState Simplification (Massive), Michael S. Tsirkin, 2014/04/27
- Re: [Qemu-devel] [PATCH v2 000/124] VMState Simplification (Massive), Juan Quintela, 2014/04/21
[Qemu-devel] [PATCH 115/124] vmstate: Test for VMSTATE_STRUCT_POINTER, Juan Quintela, 2014/04/21
[Qemu-devel] [PATCH 071/124] vmstate: Remove version field from VMSTATE_STRUCT_VARRAY_UINT32, Juan Quintela, 2014/04/21
[Qemu-devel] [PATCH 116/124] vmstate: Test VMSTATE_STRUCT_ARRAY, Juan Quintela, 2014/04/21