Message ID | 1398091304-10677-1-git-send-email-quintela@redhat.com |
---|---|
State | New |
Headers | show |
On 21 April 2014 15:39, Juan Quintela <quintela@redhat.com> wrote: > - version_mimium_id_old patches splitted as for Peter requests in: > * usb > * x86 > * arm > * ppc > * rest > > I splitted basically on that order, so, if a device appears on more > than one architecture, 1st one on the list wins Thanks. How about you get those patches reviewed and in first, rather than including them in a 124 patch series which touches 238 files? I just don't think this is reviewable at all, even at a very high level (I can't tell from diffstat if you're touching all these files just for the removal of the version-minimum-id-old field or for some other thing too, for instance.) I think you need to divide this up into different series and send them through the review-and-commit pipeline separately. thanks -- PMM
* Paolo Bonzini (pbonzini@redhat.com) wrote: > Il 21/04/2014 13:34, Peter Maydell ha scritto: > >Mostly just that I think that for vmstate definitions "this new field > >was added in version X" is natural and normal, whereas other > >test functions are odd and generally the exception. So a simple > >way to indicate minimum version for fields seems useful to retain. > >I don't mind if you want to unify the underlying implementation, > >but I would prefer to retain the _V macros for vmstate definitions > >to use (and it has the additional advantage of avoiding the need for > >touching lots of devices...) > > I agree. In many cases, _TEST is a huge review warning sign that > subsections should have been used instead. I can see how the subsections should be used in some cases, but I've come across at least one case where _TEST was used to avoid the need for a version change. Mst's 9e047b (hw/acpi/piix4.c) replaces an existing field, if a property on the device is set, but if the property is as-before then the structure stays exactly as it was. I can see how that probably should have used a subsection for the new version of the data, but I don't see how it could have otherwise kept it's compatibility. Dave -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
Il 22/04/2014 13:39, Dr. David Alan Gilbert ha scritto: >> > >> > I agree. In many cases, _TEST is a huge review warning sign that >> > subsections should have been used instead. > I can see how the subsections should be used in some cases, but I've > come across at least one case where _TEST was used to avoid the need > for a version change. > > Mst's 9e047b (hw/acpi/piix4.c) replaces an existing field, if a property > on the device is set, but if the property is as-before then the structure > stays exactly as it was. > I can see how that probably should have used a subsection for the new > version of the data, but I don't see how it could have otherwise kept > it's compatibility. Was there really any need to remove the existing field? Could you simply have its contents (probably all zeroes) streamed anyway? (Reminds me of "#define union struct /* Wastes some memory */" :)). Paolo
On Sun, Apr 27, 2014 at 10:26:30AM +0200, Paolo Bonzini wrote: > Il 22/04/2014 13:39, Dr. David Alan Gilbert ha scritto: > >>> > >>> I agree. In many cases, _TEST is a huge review warning sign that > >>> subsections should have been used instead. > >I can see how the subsections should be used in some cases, but I've > >come across at least one case where _TEST was used to avoid the need > >for a version change. > > > >Mst's 9e047b (hw/acpi/piix4.c) replaces an existing field, if a property > >on the device is set, but if the property is as-before then the structure > >stays exactly as it was. > >I can see how that probably should have used a subsection for the new > >version of the data, but I don't see how it could have otherwise kept > >it's compatibility. > > Was there really any need to remove the existing field? Could you > simply have its contents (probably all zeroes) streamed anyway? > > (Reminds me of "#define union struct /* Wastes some memory */" :)). > > Paolo Sure, we could do this, I just didn't see the advantage of that and it seemed cleaner.