mbox

[v2,000/124] VMState Simplification (Massive)

Message ID 1398091304-10677-1-git-send-email-quintela@redhat.com
State New
Headers show

Pull-request

git://github.com/juanquintela/qemu.git tags/vmstate-simplification/20140421

Message

Juan Quintela April 21, 2014, 2:39 p.m. UTC
Hi

New on v2:
- new testing harness
  Now tests are smaller, and i would claim easier to understand

- now all things exported for vmstate.h are tested

  structs/pointers/arrays and all combinations

  ok, I am lying VMSTATE_OPENCODED_UNSAFE is not tested for obvious
  reasons

- 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

- Fix for vmstate core error was not enough.  If we find one error,
  set qemu_error.

- Refactor object creation during the tests.  We want to reset
  destination "objects" when testing more than once.  I was getting
  random errors because I assumed that un-init data was different from
  init data (yes, I know ...)

  First, I try the old :memset(foo, 0, sizeof(foo)), but that don't
  work very well when you have pointers, so created functions that
  reset correctly them.

- Make checkpatch as happy as possible

  You know that you have tried too much when you find bugs on
  checkpatch itself, see my mail about that on the mailing list.

- Make version functions static on vmstate.c instead of inline to
  allow for testing (Andreas request).


And that is it.

ToDo:

- Simplify the generation of synthetic values, and use that to
  implement _equal(), _vmstate and VMSTATE_VALIDATE().

  But this would be a different series.

Please review.

Later, Juan.

PD. Yes, rebasing (continously) a tree with 124 patches is an exercise in pain :-(


[v1]
Hi

  Look at the diffstat.  Almost all the additions are at
test-vmstate.c. That is the reason why it is called a simplification.

What this series does:
- peter removal of version_minimum_id_old field when not needed (Peter)
- cleanup: based on the previous one, I removed all the unneeded
  the uses on the tree.  This should make your compiles
  a couple of nanoseconds faster.
- once there, fixed the indentation of the .fields line, to a canonical
    .fields = (VMStateField[])
- mst simplifications for vmstate engine

And now, the big cleanup.
- Patches only do one thing, to make easy the review.

- Added test for all VMSTATE_FOO() definitions
  (well, I am lying, VMSTATE_STRUCT* are still missing, will come soon)
- We had two ways to make a field optional
   VMSTATE_INT64_V(field, state, version)
  and
   VMSTATE_INT64_TEST(field, state, test)

  We can do the version one with one test like:

static inline bool vmstate_5_plus(void *opaque, int version_id)
{
    return version_id >= 5;
}

  and then change:
  VMSTATE_INT64_V(field, state, 5);

  into
  VMSTATE_INT64_TEST(field, state, vmstate_5_plus);

  The functions until version 15 are already defined on the tree.
  All the users on the tree are changed to this new schema.  Actually we used
  to have:
  VMSTATE_FOO()
  VMSTATE_FOO_V()
  VMSTATE_FOO_TEST()

  Where some of them where defined.  After all the changes, only exst
  VMSTATE_FOO()
  VMSTATE_FOO_TEST()   (When needed)

- When all the changes are done, removal from version_id at the
  FieldLevel is removed.  Notice that from the toplevel, all the
  versions work the same at the section level, the only thing that has
  changed is at the field level.

- Doing the tests, I found a bug (tests have some utility after all)

  When the incoming migration stream stoped for any reason, we setup
  an error, but until we don't end a section, we never check for one
  error, so it could take a lot until we found it.  Now we check after
  each field read.

- You can see that all the tests share a lot of the boilerplate code.
  If you have any good idea about how to refactor it to not repeat so
  much code, let me know.

- Remove all the VMSTATE_FOO* that had no users

- Create new ones for things that were opencoded

  VMSTATE_SYNTHETIC(): fields that don't exist on the state struct and
  are generated It was already opencoded, just created a macro and
  used it where needed.

  VMSTATE_OPENCODED_UNSAFE: There are a couple of cases where a non
  vmstate field is needed from vmstate.  We can cheat and declare that
  as an struct, but that is not vmstate.  There were already users on
  the tree, so create it.

If you ever wanted to know more about VMState, this is your
opportunity.  As the tests are aded one field at a time, you can see
how the things on the wire change each time to add a new field.

ToDo:
- Finish the tests for the STRUCTS and ARRAYS of STRUCTS of several kinds
- synthetic fields: we can do even better/easier.  We can add a hook
  for the cases where we want to compute the value in a different way.

  This could be used for the validate stuff that Michael requested on
  its patches, while allowing to simplify even more tests.

- port the _vmstate fields to the new synthetic framework.

Thanks for reading until here.

Later, Juan.
[end v1]



The following changes since commit 2d03b49c3f225994c4b0b46146437d8c887d6774:

  Merge remote-tracking branch 'remotes/pmaydell/tags/pull-target-arm-20140417-1' into staging (2014-04-17 21:37:26 +0100)

are available in the git repository at:


  git://github.com/juanquintela/qemu.git tags/vmstate-simplification/20140421

for you to fetch changes up to d587ef5f4657c9c6ba26fe27fc2d78a0d051e30f:

  vmstate: Test for VMSTATE_ARRAY_OF_POINTER (2014-04-21 16:31:22 +0200)

----------------------------------------------------------------
vmstate-simplification/next for 20140421

----------------------------------------------------------------
Juan Quintela (121):
      savevm: Remove all the unneded version_minimum_id_old (usb)
      savevm: Remove all the unneded version_minimum_id_old (ppc)
      savevm: Remove all the unneded version_minimum_id_old (arm)
      savevm: Remove all the unneded version_minimum_id_old (x86)
      savevm: Remove all the unneded version_minimum_id_old (rest)
      vmstate: Return error in case of error
      vmstate: Refactor opening of files
      vmstate: Refactor & increase tests for primitive types
      vmstate: Port versioned tests to new format
      vmstate: Create test functions for versions until 15
      vmstate: Remove VMSTATE_UINTL_EQUAL_V
      vmstate: Change VMSTATE_INTTL_V to VMSTATE_INTTL_TEST
      vmstate: Remove unused VMSTATE_UINTTL_ARRAY_V
      vmstate: Test for VMSTATE_BOOL_TEST
      vmstate: Test for VMSTATE_INT8_TEST
      vmstate: Test for VMSTATE_INT16_TEST
      vmstate: Test for VMSTATE_INT32_TEST
      vmstate: test for VMSTATE_INT64_TEST
      vmstate: Test for VMSTATE_UINT8_TEST
      vmstate: Test for VMSTATE_UINT16_TEST
      vmstate: Test for VMSTATE_UINT32_TEST
      vmstate: Test for VMSTATE_UINT64_TEST
      vmstate: Test for VMSTATE_FLOAT64
      vmstate: Test for VMSTATE_UNUSED
      vmstate: Test for VMSTATE_BITMAP
      vmstate: Test for VMSTATE_UINT8_EQUAL
      vmstate: Test for VMSTATE_UINT16_EQUAL
      vmstate: Test for VMSTATE_UINT32_EQUAL
      vmstate: Test for VMSTATE_UINT64_EQUAL
      vmstate: Test for VMSTATE_INT32_EQUAL
      vmstate: Test for VMSTATE_INT32_LE
      vmstate: Move VMSTATE_TIMER_V to VMSTATE_TIMER_TEST
      vmstate: Test for VMSTATE_ARRAY_BOOL_TEST
      vmstate: Test for VMSTATE_UINT8_ARRAY
      vmstate: Test for VMSTATE_UINT16_ARRAY
      vmstate: Test for VMSTATE_UINT32_ARRAY{_TEST}
      vmstate: Test for VMSTATE_UINT64_ARRAY{_TEST}
      vmstate: Test for VMSTATE_INT16_ARRAY
      vmstate: Test for VMSTATE_INT32_ARRAY{_TEST}
      vmstate: Test for VMSTATE_INT64_ARRAY
      vmstate: Test for VMSTATE_FLOAT64_ARRAY
      vmstate: Test for VMSTATE_UINT8_2DARRAY
      vmstate: Test for VMSTATE_UINT16_2DARRAY.
      vmstate: Test for VMSTATE_UINT32_2DARRAY
      vmstate: Remove unused VMSTATE_BUFFER_V
      vmstate: Remove version from VMSTATE_BUFFER_UNSAFE
      vmstate: Remove unused version fields from ARM
      vmstate: All ptimers users were at least at version 1 or more
      vmstate: Remove version from all variants of VMSTATE_STRUCT_POINTER*
      vmstate: Port last 3 users of VMSTATE_ARRAY to VMSTATE_ARRAY_TEST
      vmstate: Port last user of VMSTATE_SINGLE to VMSTATE_SINGLE_TEST
      vmstate: Remove unused VMSTATE_POINTER
      vmstate: Rename VMSTATE_SINGLE_TEST to VMSTATE_SINGLE
      vmstate: Move VMSTATE_2DARRAY to use _test
      vmstate: Rename VMSTATE_POINTER_TEST without _TEST
      vmstate: Rename VMSTATE_ARRAY_TEST to VMSTATE_ARRAY
      vmstate: Remove version_id from VMSTATE_VBUFFER
      vmstate: Remove version_id fields that were not used
      vmstate: Remove version_id from VMSTATE_SUB_ARRAY
      vmstate: Remove version parameter from VMSTATE_VARRAY_INT32
      vmstate: Remove version_id from VMSTATE_VARRAY_UINT16_UNSAFE
      vmstate: Remove unused version_id from VMSTATE_ARRAY_OF_POINTER
      vmstate: remove version parameter from VMSTATE_BUFFER_POINTER_UNSAFE
      vmstate: Remove version, test and start parameter from VMSTATE_VBUFFER_UINT32
      vmstate: Remove version paramenter from VMSTATE_ARRAY_OF_POINTER_TO_STRUCT
      vmstate: Remove VMSTATE_BUFFER_MULTIPLY
      vmstate: Remove version parameter from VMSTATE_STATIC_BUFFER
      vmstate: Remove version field from VMSTATE_STRUCT_VARRAY_UINT32
      vmstate: Move all users of versioning of VMSTATE_STRUCT_ARRAY to _TEST
      vmstate: Remove version parameter from VMSTATE_STRUCT_ARRAY
      vmstate: Remove version parameter from VMSTATE_STRUCT_ARRAY_TEST
      vmstate: Remove unused version parameter from VMSTATE_STRUCT_VARRAY_INT32
      vmstate: Remove unused version parameter from VMSTATE_STRUCT_VARRAY_UINT8
      vmstate: Introduce VMSTATE_VARRAY_UINT32_TEST
      vmstate: Remove version parameter from VMSTATE_VARRAY_UINT32
      vmstate: Remove version parameter from VMSTATE_STRUCT_TEST
      vmstate: Move all users of versioning to VMSTATE_STRUCT_TEST
      vmstate: Remove version from all VMSTATE_STRUCT calls
      vmstate: Create VMSTATE_VARRAY macro
      vmstate: Create VMSTATE_POINTER_UNSAFE
      vmstate: Create VMSTATE_OPENCODED_UNSAFE
      vmstate: Create VMSTATE_SYNTHETIC
      vmstate: version_id is gone from fields
      vmstate: Test for VMSTATE_SYNTHETIC
      vmstate: Test for VMSTATE_UINT8_SUB_ARRAY
      vmstate: Test for VMSTATE_UINT32_SUB_ARRAY
      vmstate: Test for VMSTATE_BUFFER
      vmstate: Test for VMSTATE_PARTIAL_BUFFER
      vmstate: Test for VMSTATE_BUFFER_START_MIDDLE
      vmstate: Test for VMSTATE_BUFFER_TEST
      vmstate: Use VMSTATE_UINT8_2DARRAY instead of VMSTATE_BUFFER_TEST
      vmstate: Test for VMSTATE_BUFFER_UNSAFE
      vmstate: Remove unused VMSTATE_SUB_VBUFFER
      vmstate: Remove unused VMSTATE_PARTIAL_VBUFFER_UINT32
      vmstate: Test for VMSTATE_PARTIAL_VBUFFER
      vmstate: Rename VMSTATE_PARTIAL_VBUFFER to VMSTATE_VBUFFER_INT32
      vmstate: Create VMS_VBUFFER_UINT32
      vmstate: Rename VMS_VBUFFER to VMST_VBUFFER_INT32 for consintency
      vmstate: Test for VMSTATE_VBUFFER_UINT32
      vmstate: VMSTATE_POINTER() used the wrong type to calculate the size
      vmstate: Test for VMSTATE_POINTER
      vmstate: Test for VMSTATE_POINTER_UNSAFE
      vmstate: Test for VMSTATE_BUFFER_UNSAFE_TEST
      vmstate: Test for VMSTATE_BUFFER_POINTER_UNSAFE
      vmstate: Test for VMSTATE_ARRAY_INT32_UNSAFE
      vmstate: Test for VMSTATE_VARRAY
      vmstate: Test for VMSTATE_VARRAY_INT32
      vmstate: Test for VMSTATE_VARRAY_UINT16_UNSAFE
      vmstate: Test for VMSTATE_VARRAY_INT32{_TEST}
      vmstate: Test for VMSTATE_STRUCT{_TEST}
      vmstate: Remove unused VMSTATE_STRUCT_POINTER_TEST
      vmstate: Test for VMSTATE_STRUCT_POINTER
      vmstate: Test VMSTATE_STRUCT_ARRAY
      vmstate: Test for VMSTATE_STRUCT_VARRAY_UINT8
      vmstate: Test for VMSTATE_STRUCT_VARRAY_UINT32
      vmstate: Test for VMSTATE_STRUCT_VARRAY_INT32
      vmstate: Test for VMSTATE_STRUCT_VARRAY_POINTER_UINT16
      vmstate: Test for VMSTATE_STRUCT_ARRAY_POINTER_UINT32
      vmstate: Test for VMSTATE_STRUCT_VARRAY_POINTER_INT32
      vmstate: Test for VMSTATE_ARRAY_OF_POINTER_TO_STRUCT
      vmstate: Test for VMSTATE_ARRAY_OF_POINTER

Michael S. Tsirkin (2):
      vmstate: Reduce code duplication
      vmstate: s/VMSTATE_INT32_LE/VMSTATE_INT32_POSITIVE_LE/

Peter Maydell (1):
      savevm: Ignore minimum_version_id_old if there is no load_state_old

 audio/audio.c                      |    3 +-
 cpus.c                             |    5 +-
 docs/migration.txt                 |   20 +-
 exec.c                             |    3 +-
 hw/acpi/ich9.c                     |   12 +-
 hw/acpi/pcihp.c                    |    3 +-
 hw/acpi/piix4.c                    |   29 +-
 hw/arm/highbank.c                  |    1 -
 hw/arm/musicpal.c                  |   10 +-
 hw/arm/pxa2xx.c                    |   19 +-
 hw/arm/pxa2xx_gpio.c               |    3 +-
 hw/arm/pxa2xx_pic.c                |    1 -
 hw/arm/spitz.c                     |   14 +-
 hw/arm/stellaris.c                 |   14 +-
 hw/arm/strongarm.c                 |    6 -
 hw/arm/z2.c                        |    2 -
 hw/audio/ac97.c                    |    8 +-
 hw/audio/cs4231.c                  |    3 +-
 hw/audio/cs4231a.c                 |    3 +-
 hw/audio/es1370.c                  |    8 +-
 hw/audio/gus.c                     |    3 +-
 hw/audio/hda-codec.c               |   10 +-
 hw/audio/intel-hda.c               |    7 +-
 hw/audio/lm4549.c                  |    5 +-
 hw/audio/marvell_88w8618.c         |    1 -
 hw/audio/milkymist-ac97.c          |    3 +-
 hw/audio/pl041.c                   |   25 +-
 hw/audio/sb16.c                    |    3 +-
 hw/audio/wm8750.c                  |    3 +-
 hw/block/ecc.c                     |    3 +-
 hw/block/fdc.c                     |   24 +-
 hw/block/m25p80.c                  |    1 -
 hw/block/nand.c                    |    3 +-
 hw/block/onenand.c                 |    5 +-
 hw/char/cadence_uart.c             |    1 -
 hw/char/digic-uart.c               |    1 -
 hw/char/escc.c                     |   10 +-
 hw/char/exynos4210_uart.c          |    6 +-
 hw/char/imx_serial.c               |    1 -
 hw/char/ipoctal232.c               |   13 +-
 hw/char/lm32_juart.c               |    3 +-
 hw/char/lm32_uart.c                |    3 +-
 hw/char/milkymist-uart.c           |    3 +-
 hw/char/pl011.c                    |    3 +-
 hw/char/sclpconsole-lm.c           |    3 +-
 hw/char/sclpconsole.c              |    3 +-
 hw/char/serial-isa.c               |    2 +-
 hw/char/serial-pci.c               |    8 +-
 hw/char/serial.c                   |    6 +-
 hw/char/spapr_vty.c                |    3 +-
 hw/core/ptimer.c                   |    3 +-
 hw/display/ads7846.c               |    3 +-
 hw/display/cg3.c                   |    2 +-
 hw/display/cirrus_vga.c            |    8 +-
 hw/display/exynos4210_fimd.c       |    6 +-
 hw/display/g364fb.c                |    7 +-
 hw/display/jazz_led.c              |    1 -
 hw/display/milkymist-tmu2.c        |    3 +-
 hw/display/milkymist-vgafb.c       |    3 +-
 hw/display/pl110.c                 |    2 +-
 hw/display/pxa2xx_lcd.c            |    8 +-
 hw/display/qxl.c                   |    8 +-
 hw/display/ssd0303.c               |    3 +-
 hw/display/tcx.c                   |    3 +-
 hw/display/vga-pci.c               |    5 +-
 hw/display/vga.c                   |    3 +-
 hw/display/vmware_vga.c            |   10 +-
 hw/dma/i82374.c                    |    2 +-
 hw/dma/i8257.c                     |    9 +-
 hw/dma/pl080.c                     |    2 +-
 hw/dma/pl330.c                     |   25 +-
 hw/dma/pxa2xx_dma.c                |    2 -
 hw/dma/sparc32_dma.c               |    3 +-
 hw/dma/sun4m_iommu.c               |    3 +-
 hw/gpio/max7310.c                  |    3 +-
 hw/gpio/pl061.c                    |    2 +-
 hw/gpio/zaurus.c                   |    3 +-
 hw/i2c/core.c                      |    6 +-
 hw/i2c/smbus_ich9.c                |    1 -
 hw/i386/acpi-build.c               |    3 +-
 hw/i386/kvm/clock.c                |    1 -
 hw/i386/kvmvapic.c                 |    9 +-
 hw/i386/pc.c                       |    3 +-
 hw/ide/ahci.c                      |    6 +-
 hw/ide/core.c                      |   20 +-
 hw/ide/ich.c                       |    2 +-
 hw/ide/internal.h                  |    7 +-
 hw/ide/isa.c                       |    3 +-
 hw/ide/macio.c                     |    3 +-
 hw/ide/microdrive.c                |    3 +-
 hw/ide/mmio.c                      |    3 +-
 hw/ide/pci.c                       |   15 +-
 hw/input/adb.c                     |    6 +-
 hw/input/hid.c                     |    2 +-
 hw/input/lm832x.c                  |    3 +-
 hw/input/milkymist-softusb.c       |    3 +-
 hw/input/pckbd.c                   |    8 +-
 hw/input/ps2.c                     |   18 +-
 hw/input/pxa2xx_keypad.c           |    3 +-
 hw/input/stellaris_input.c         |    8 +-
 hw/input/vmmouse.c                 |    3 +-
 hw/intc/allwinner-a10-pic.c        |    1 -
 hw/intc/arm_gic_common.c           |    2 +-
 hw/intc/armv7m_nvic.c              |    3 +-
 hw/intc/exynos4210_combiner.c      |    4 +-
 hw/intc/exynos4210_gic.c           |    3 +-
 hw/intc/heathrow_pic.c             |    8 +-
 hw/intc/i8259_common.c             |    1 -
 hw/intc/imx_avic.c                 |    1 -
 hw/intc/ioapic_common.c            |    6 +-
 hw/intc/lm32_pic.c                 |    3 +-
 hw/intc/slavio_intctl.c            |    8 +-
 hw/intc/xics.c                     |    9 +-
 hw/ipack/ipack.c                   |    3 +-
 hw/ipack/tpci200.c                 |    3 +-
 hw/isa/apm.c                       |    1 -
 hw/isa/lpc_ich9.c                  |    5 +-
 hw/isa/piix4.c                     |    3 +-
 hw/isa/vt82c686.c                  |    8 +-
 hw/misc/arm_sysctl.c               |   18 +-
 hw/misc/eccmemctl.c                |    3 +-
 hw/misc/exynos4210_pmu.c           |    2 +-
 hw/misc/imx_ccm.c                  |    1 -
 hw/misc/lm32_sys.c                 |    3 +-
 hw/misc/macio/cuda.c               |    8 +-
 hw/misc/macio/mac_dbdma.c          |    8 +-
 hw/misc/max111x.c                  |    3 +-
 hw/misc/milkymist-hpdmc.c          |    3 +-
 hw/misc/milkymist-pfpu.c           |    3 +-
 hw/misc/mst_fpga.c                 |   11 +-
 hw/misc/slavio_misc.c              |    3 +-
 hw/misc/tmp105.c                   |    3 +-
 hw/misc/zynq_slcr.c                |    3 +-
 hw/net/allwinner_emac.c            |    4 +-
 hw/net/cadence_gem.c               |    3 +-
 hw/net/e1000.c                     |    6 +-
 hw/net/eepro100.c                  |    3 +-
 hw/net/lan9118.c                   |   20 +-
 hw/net/lance.c                     |    5 +-
 hw/net/milkymist-minimac2.c        |    8 +-
 hw/net/mipsnet.c                   |    3 +-
 hw/net/ne2000-isa.c                |    5 +-
 hw/net/ne2000.c                    |   10 +-
 hw/net/pcnet-pci.c                 |    5 +-
 hw/net/pcnet.c                     |    3 +-
 hw/net/rtl8139.c                   |   13 +-
 hw/net/smc91c111.c                 |    4 +-
 hw/net/spapr_llan.c                |    3 +-
 hw/net/vmxnet3.c                   |   13 +-
 hw/net/xgmac.c                     |    4 +-
 hw/nvram/ds1225y.c                 |    3 +-
 hw/nvram/eeprom93xx.c              |   14 +-
 hw/nvram/fw_cfg.c                  |    7 +-
 hw/nvram/mac_nvram.c               |    5 +-
 hw/pci-bridge/ioh3420.c            |    3 +-
 hw/pci-bridge/xio3130_downstream.c |    3 +-
 hw/pci-bridge/xio3130_upstream.c   |    3 +-
 hw/pci-host/bonito.c               |    3 +-
 hw/pci-host/piix.c                 |   11 +-
 hw/pci-host/ppce500.c              |   13 +-
 hw/pci-host/q35.c                  |    3 +-
 hw/pci/msix.c                      |   10 +-
 hw/pci/pci.c                       |   26 +-
 hw/pci/pcie_aer.c                  |    6 +-
 hw/ppc/ppc4xx_pci.c                |   19 +-
 hw/ppc/spapr.c                     |    3 +-
 hw/ppc/spapr_iommu.c               |    6 +-
 hw/ppc/spapr_pci.c                 |   13 +-
 hw/ppc/spapr_vio.c                 |    3 +-
 hw/s390x/event-facility.c          |    3 +-
 hw/s390x/sclpquiesce.c             |    3 +-
 hw/scsi/esp-pci.c                  |    5 +-
 hw/scsi/esp.c                      |    6 +-
 hw/scsi/lsi53c895a.c               |    7 +-
 hw/scsi/megasas.c                  |    3 +-
 hw/scsi/scsi-bus.c                 |   14 +-
 hw/scsi/scsi-disk.c                |    1 -
 hw/scsi/spapr_vscsi.c              |    6 +-
 hw/scsi/vmw_pvscsi.c               |    3 +-
 hw/sd/milkymist-memcard.c          |    3 +-
 hw/sd/sd.c                         |    4 +-
 hw/sd/sdhci.c                      |    4 +-
 hw/ssi/pl022.c                     |    3 +-
 hw/ssi/ssi.c                       |    3 +-
 hw/ssi/xilinx_spi.c                |    1 -
 hw/ssi/xilinx_spips.c              |    1 -
 hw/timer/a9gtimer.c                |    2 +-
 hw/timer/allwinner-a10-pit.c       |    1 -
 hw/timer/arm_mptimer.c             |    2 +-
 hw/timer/arm_timer.c               |    6 +-
 hw/timer/cadence_ttc.c             |    5 +-
 hw/timer/digic-timer.c             |    1 -
 hw/timer/ds1338.c                  |    3 +-
 hw/timer/exynos4210_mct.c          |   19 +-
 hw/timer/exynos4210_pwm.c          |    6 +-
 hw/timer/exynos4210_rtc.c          |    1 -
 hw/timer/hpet.c                    |   13 +-
 hw/timer/i8254_common.c            |    9 +-
 hw/timer/imx_epit.c                |    3 +-
 hw/timer/imx_gpt.c                 |    3 +-
 hw/timer/lm32_timer.c              |    3 +-
 hw/timer/m48t59.c                  |    5 +-
 hw/timer/mc146818rtc.c             |   17 +-
 hw/timer/milkymist-sysctl.c        |    3 +-
 hw/timer/pxa2xx_timer.c            |   13 +-
 hw/timer/slavio_timer.c            |    8 +-
 hw/timer/twl92230.c                |   14 +-
 hw/usb/bus.c                       |    2 +-
 hw/usb/dev-hid.c                   |    4 +-
 hw/usb/dev-hub.c                   |    6 +-
 hw/usb/dev-smartcard-reader.c      |    6 +-
 hw/usb/dev-storage.c               |    2 +-
 hw/usb/hcd-ehci-pci.c              |    5 +-
 hw/usb/hcd-ehci-sysbus.c           |    5 +-
 hw/usb/hcd-ehci.c                  |    6 +-
 hw/usb/hcd-uhci.c                  |   12 +-
 hw/usb/hcd-xhci.c                  |   10 +-
 hw/usb/redirect.c                  |   40 +-
 hw/watchdog/wdt_i6300esb.c         |   14 +-
 hw/watchdog/wdt_ib700.c            |    3 +-
 hw/xen/xen_platform.c              |    3 +-
 include/hw/acpi/pcihp.h            |    2 +-
 include/hw/hw.h                    |   31 +-
 include/hw/ipack/ipack.h           |    2 +-
 include/hw/pci/shpc.h              |    2 +-
 include/hw/ppc/spapr_vio.h         |    2 +-
 include/hw/ptimer.h                |    4 +-
 include/migration/vmstate.h        |  471 +++----
 target-alpha/machine.c             |   20 +-
 target-arm/machine.c               |   29 +-
 target-i386/machine.c              |  160 ++-
 target-lm32/machine.c              |    8 +-
 target-moxie/machine.c             |    3 +-
 target-openrisc/machine.c          |    4 +-
 target-ppc/machine.c               |   52 +-
 tests/test-vmstate.c               | 2450 ++++++++++++++++++++++++++++++++----
 util/fifo8.c                       |    5 +-
 vmstate.c                          |  141 ++-
 238 files changed, 3107 insertions(+), 1617 deletions(-)

Comments

Peter Maydell April 21, 2014, 4:10 p.m. UTC | #1
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
Dr. David Alan Gilbert April 22, 2014, 11:39 a.m. UTC | #2
* 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
Paolo Bonzini April 27, 2014, 8:26 a.m. UTC | #3
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
Michael S. Tsirkin April 27, 2014, 11:19 a.m. UTC | #4
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.