migration: let pre_save fail
cover migration: let pre_save fail
From: "Dr. David Alan Gilbert" <[email protected]>
At the moment the pre-save method on VMStateDescription returns void which means there's no sane way of a pre_save routine failing. There are quite a few pre_save's that abort or do other nasty things, and it would be better if they could fail cleanly.
This patch series doesn't fix any of those pre_save's - it just changes pre_save to return an error value and routes that back up so the migration fails. I'll leave that up to the individual device owners.
There are a bunch of corner cases that I've not fixed yet; these are mostly places that call vmstate_save_state which have been partially converted to modern migration code; often the functions they're called from return void as well - they can get fixed later.
Dr. David Alan Gilbert (5):
migration: pre_save return int
migration: check pre_save return in vmstate_save_state
migration: wire vmstate_save_state errors up to
vmstate_subsection_save
migration: Route errors up through vmstate_save
migration: Route more error paths
docs/devel/migration.txt | 2 +- hw/arm/pxa2xx.c | 4 +++- hw/arm/strongarm.c | 4 +++- hw/audio/wm8750.c | 4 +++- hw/block/fdc.c | 4 +++- hw/block/m25p80.c | 4 +++- hw/block/nand.c | 4 +++- hw/block/onenand.c | 4 +++- hw/char/serial.c | 4 +++- hw/display/qxl.c | 4 +++- hw/display/virtio-gpu.c | 4 +--- hw/i2c/core.c | 4 +++- hw/i386/kvm/clock.c | 4 +++- hw/ide/core.c | 4 +++- hw/ide/pci.c | 4 +++- hw/input/ps2.c | 8 ++++++-- hw/input/tsc210x.c | 4 +++- hw/intc/apic_common.c | 4 +++- hw/intc/arm_gic_common.c | 4 +++- hw/intc/arm_gicv3_common.c | 4 +++- hw/intc/arm_gicv3_its_common.c | 4 +++- hw/intc/i8259_common.c | 4 +++- hw/intc/ioapic_common.c | 4 +++- hw/intc/s390_flic_kvm.c | 6 ++++-- hw/intc/xics.c | 8 ++++++-- hw/net/e1000.c | 4 +++- hw/net/e1000e.c | 4 +++- hw/net/rtl8139.c | 4 +++- hw/net/virtio-net.c | 16 ++++++++++++---- hw/net/vmxnet3.c | 4 +++- hw/pci-host/piix.c | 4 +++- hw/ppc/ppc.c | 4 +++- hw/ppc/spapr_iommu.c | 4 +++- hw/ppc/spapr_pci.c | 6 ++++-- hw/s390x/css.c | 10 +++++++--- hw/s390x/virtio-ccw.c | 4 +++- hw/scsi/lsi53c895a.c | 4 +++- hw/scsi/vmw_pvscsi.c | 4 +++- hw/timer/cadence_ttc.c | 4 +++- hw/timer/hpet.c | 4 +++- hw/timer/i8254_common.c | 4 +++- hw/timer/mc146818rtc.c | 4 +++- hw/timer/pl031.c | 4 +++- hw/timer/twl92230.c | 4 +++- hw/usb/dev-smartcard-reader.c | 4 +++- hw/usb/hcd-ehci.c | 4 +++- hw/usb/redirect.c | 4 +++- hw/virtio/vhost-vsock.c | 4 +++- hw/virtio/virtio.c | 13 +++++++------ include/hw/virtio/virtio.h | 2 +- include/migration/vmstate.h | 6 +++--- migration/colo-comm.c | 4 +++- migration/global_state.c | 4 +++- migration/savevm.c | 23 +++++++++++++++++------ migration/trace-events | 1 + migration/vmstate-types.c | 11 ++++++++--- migration/vmstate.c | 34 +++++++++++++++++++++++++--------- replay/replay-snapshot.c | 4 +++- slirp/slirp.c | 8 ++++++-- target/arm/machine.c | 4 +++- target/i386/machine.c | 7 +++++-- target/ppc/machine.c | 4 +++- target/s390x/machine.c | 4 +++- target/sparc/machine.c | 4 +++- tests/test-vmstate.c | 10 ++++++---- 65 files changed, 261 insertions(+), 102 deletions(-)
Signature · 1 line
2.13.5
PATCH 1/5 migration: pre_save return int
From: "Dr. David Alan Gilbert" <[email protected]>
Modify the pre_save method on VMStateDescription to return an int rather than void so that it potentially can fail.
Changed zillions of devices to make them return 0; the only case I've made it return non-0 is hw/intc/s390_flic_kvm.c that already had an error_report/return case.
Note: If you add an error exit in your pre_save you must emit an error_report to say why.
--- docs/devel/migration.txt | 2 +- hw/arm/pxa2xx.c | 4 +++- hw/arm/strongarm.c | 4 +++- hw/audio/wm8750.c | 4 +++- hw/block/fdc.c | 4 +++- hw/block/m25p80.c | 4 +++- hw/block/nand.c | 4 +++- hw/block/onenand.c | 4 +++- hw/char/serial.c | 4 +++- hw/display/qxl.c | 4 +++- hw/i2c/core.c | 4 +++- hw/i386/kvm/clock.c | 4 +++- hw/ide/core.c | 4 +++- hw/ide/pci.c | 4 +++- hw/input/ps2.c | 8 ++++++-- hw/input/tsc210x.c | 4 +++- hw/intc/apic_common.c | 4 +++- hw/intc/arm_gic_common.c | 4 +++- hw/intc/arm_gicv3_common.c | 4 +++- hw/intc/arm_gicv3_its_common.c | 4 +++- hw/intc/i8259_common.c | 4 +++- hw/intc/ioapic_common.c | 4 +++- hw/intc/s390_flic_kvm.c | 6 ++++-- hw/intc/xics.c | 8 ++++++-- hw/net/e1000.c | 4 +++- hw/net/e1000e.c | 4 +++- hw/net/rtl8139.c | 4 +++- hw/net/virtio-net.c | 16 ++++++++++++---- hw/net/vmxnet3.c | 4 +++- hw/pci-host/piix.c | 4 +++- hw/ppc/ppc.c | 4 +++- hw/ppc/spapr_iommu.c | 4 +++- hw/ppc/spapr_pci.c | 6 ++++-- hw/s390x/css.c | 10 +++++++--- hw/s390x/virtio-ccw.c | 4 +++- hw/scsi/lsi53c895a.c | 4 +++- hw/scsi/vmw_pvscsi.c | 4 +++- hw/timer/cadence_ttc.c | 4 +++- hw/timer/hpet.c | 4 +++- hw/timer/i8254_common.c | 4 +++- hw/timer/mc146818rtc.c | 4 +++- hw/timer/pl031.c | 4 +++- hw/timer/twl92230.c | 4 +++- hw/usb/dev-smartcard-reader.c | 4 +++- hw/usb/hcd-ehci.c | 4 +++- hw/usb/redirect.c | 4 +++- hw/virtio/vhost-vsock.c | 4 +++- include/migration/vmstate.h | 2 +- migration/colo-comm.c | 4 +++- migration/global_state.c | 4 +++- migration/savevm.c | 4 +++- replay/replay-snapshot.c | 4 +++- slirp/slirp.c | 8 ++++++-- target/arm/machine.c | 4 +++- target/i386/machine.c | 7 +++++-- target/ppc/machine.c | 4 +++- target/s390x/machine.c | 4 +++- target/sparc/machine.c | 4 +++- tests/test-vmstate.c | 4 +++- 59 files changed, 199 insertions(+), 70 deletions(-)
docs/devel/migration.txt +1 -1
@@ -202,7 +202,7 @@ The functions to do that are inside a vmstate definition, and are called: This function is called after we load the state of one device. -- void (*pre_save)(void *opaque); +- int (*pre_save)(void *opaque); This function is called before we save the state of one device.
hw/arm/pxa2xx.c +3 -1
@@ -1143,13 +1143,15 @@ static void pxa2xx_rtc_init(Object *obj)sysbus_init_mmio (dev, &s->iomem); } -static void pxa2xx_rtc_pre_save (void *opaque) +static int pxa2xx_rtc_pre_save (void *opaque) {PXA2xxRTCState *s = (PXA2xxRTCState *) opaque;pxa2xx_rtc_hzupdate (s);pxa2xx_rtc_piupdate (s);pxa2xx_rtc_swupdate (s); + +return 0 ; }static int pxa2xx_rtc_post_load (void *opaque,int version_id)
hw/arm/strongarm.c +3 -1
@@ -406,11 +406,13 @@ static void strongarm_rtc_init(Object *obj)sysbus_init_mmio (dev, &s->iomem); } -static void strongarm_rtc_pre_save (void *opaque) +static int strongarm_rtc_pre_save (void *opaque) {StrongARMRTCState *s = opaque;strongarm_rtc_hzupdate (s); + +return 0 ; }static int strongarm_rtc_post_load (void *opaque,int version_id)
hw/audio/wm8750.c +3 -1
@@ -567,11 +567,13 @@ static int wm8750_rx(I2CSlave *i2c)return 0x00 ; } -static void wm8750_pre_save (void *opaque) +static int wm8750_pre_save (void *opaque) {WM8750State *s = opaque; s->rate_vmstate = s->rate - wm_rate_table; + +return 0 ; }static int wm8750_post_load (void *opaque,int version_id)
hw/block/fdc.c +3 -1
@@ -1102,11 +1102,13 @@ static int reconstruct_phase(FDCtrl *fdctrl) } } -static void fdc_pre_save (void *opaque) +static int fdc_pre_save (void *opaque) {FDCtrl *s = opaque; s->dor_vmstate = s->dor |GET_CUR_DRV (s); + +return 0 ; }static int fdc_pre_load (void *opaque)
hw/block/m25p80.c +3 -1
@@ -1251,9 +1251,11 @@ static void m25p80_reset(DeviceState *d)reset_memory (s); } -static void m25p80_pre_save (void *opaque) +static int m25p80_pre_save (void *opaque) {flash_sync_dirty ((Flash *)opaque,-1 ); + +return 0 ; }static Property m25p80_properties[] = {
hw/block/nand.c +3 -1
@@ -325,11 +325,13 @@ static void nand_command(NANDFlashState *s) } } -static void nand_pre_save (void *opaque) +static int nand_pre_save (void *opaque) {NANDFlashState *s =NAND (opaque); s->ioaddr_vmstate = s->ioaddr - s->io; + +return 0 ; }static int nand_post_load (void *opaque,int version_id)
hw/block/onenand.c +3 -1
@@ -137,7 +137,7 @@ static void onenand_intr_update(OneNANDState *s)qemu_set_irq (s->intr, ((s->intstatus >>15 ) ^ (~s->config[0 ] >>6 )) &1 ); } -static void onenand_pre_save (void *opaque) +static int onenand_pre_save (void *opaque) {OneNANDState *s = opaque;if (s->current == s->otp) {
@@ -147,6 +147,8 @@ static void onenand_pre_save(void *opaque) }else { s->current_direction =0 ; } + +return 0 ; }static int onenand_post_load (void *opaque,int version_id)
hw/char/serial.c +3 -1
@@ -630,10 +630,12 @@ static void serial_event(void *opaque, int event)serial_receive_break (s); } -static void serial_pre_save (void *opaque) +static int serial_pre_save (void *opaque) {SerialState *s = opaque; s->fcr_vmstate = s->fcr; + +return 0 ; }static int serial_pre_load (void *opaque)
hw/display/qxl.c +3 -1
@@ -2204,7 +2204,7 @@ static void qxl_realize_secondary(PCIDevice *dev, Error **errp)qxl_realize_common (qxl, errp); } -static void qxl_pre_save (void *opaque) +static int qxl_pre_save (void *opaque) {PCIQXLDevice * d = opaque;uint8_t *ram_start = d->vga.vram_ptr;
@@ -2216,6 +2216,8 @@ static void qxl_pre_save(void *opaque) d->last_release_offset = (uint8_t *)d->last_release - ram_start; }assert (d->last_release_offset < d->vga.vram_size); + +return 0 ; }static int qxl_pre_load (void *opaque)
hw/i2c/core.c +3 -1
@@ -41,7 +41,7 @@ static const TypeInfo i2c_bus_info = { .instance_size = sizeof(I2CBus), }; -static void i2c_bus_pre_save (void *opaque) +static int i2c_bus_pre_save (void *opaque) {I2CBus *bus = opaque;
@@ -53,6 +53,8 @@ static void i2c_bus_pre_save(void *opaque) bus->saved_address =I2C_BROADCAST ; } } + +return 0 ; }static const VMStateDescription vmstate_i2c_bus = {
hw/i386/kvm/clock.c +3 -1
@@ -254,11 +254,13 @@ static const VMStateDescription kvmclock_reliable_get_clock = {* final pages of memory (which happens between vm_stop() * and pre_save()) takes max_downtime. */ -static void kvmclock_pre_save (void *opaque) +static int kvmclock_pre_save (void *opaque) {KVMClockState *s = opaque;kvm_update_clock (s); + +return 0 ; }static const VMStateDescription kvmclock_vmsd = {
hw/ide/core.c +3 -1
@@ -2696,7 +2696,7 @@ static int ide_drive_pio_post_load(void *opaque, int version_id)return 0 ; } -static void ide_drive_pio_pre_save (void *opaque) +static int ide_drive_pio_pre_save (void *opaque) {IDEState *s = opaque;int idx;
@@ -2712,6 +2712,8 @@ static void ide_drive_pio_pre_save(void *opaque) }else { s->end_transfer_fn_idx = idx; } + +return 0 ; }static bool ide_drive_pio_state_needed (void *opaque)
hw/ide/pci.c +3 -1
@@ -303,7 +303,7 @@ static bool ide_bmdma_status_needed(void *opaque)return ((bm->status & abused_bits) !=0 ); } -static void ide_bmdma_pre_save (void *opaque) +static int ide_bmdma_pre_save (void *opaque) {BMDMAState *bm = opaque;uint8_t abused_bits =BM_MIGRATION_COMPAT_STATUS_BITS ;
@@ -317,6 +317,8 @@ static void ide_bmdma_pre_save(void *opaque) bm->migration_retry_nsector = bm->bus->retry_nsector; bm->migration_compat_status = (bm->status & ~abused_bits) | (bm->bus->error_status & abused_bits); + +return 0 ; }/* This function accesses bm->bus->error_status which is loaded only after
hw/input/ps2.c +6 -2
@@ -1216,12 +1216,14 @@ static int ps2_kbd_post_load(void* opaque, int version_id)return 0 ; } -static void ps2_kbd_pre_save (void *opaque) +static int ps2_kbd_pre_save (void *opaque) {PS2KbdState *s = (PS2KbdState *)opaque;PS2State *ps2 = &s->common;ps2_common_post_load (ps2); + +return 0 ; }static const VMStateDescription vmstate_ps2_keyboard = {
@@ -1254,12 +1256,14 @@ static int ps2_mouse_post_load(void *opaque, int version_id)return 0 ; } -static void ps2_mouse_pre_save (void *opaque) +static int ps2_mouse_pre_save (void *opaque) {PS2MouseState *s = (PS2MouseState *)opaque;PS2State *ps2 = &s->common;ps2_common_post_load (ps2); + +return 0 ; }static const VMStateDescription vmstate_ps2_mouse = {
hw/input/tsc210x.c +3 -1
@@ -976,10 +976,12 @@ static void tsc210x_i2s_set_rate(TSC210xState *s, int in, int out) s->i2s_rx_rate = in; } -static void tsc210x_pre_save (void *opaque) +static int tsc210x_pre_save (void *opaque) {TSC210xState *s = (TSC210xState *) opaque; s->now =qemu_clock_get_ns (QEMU_CLOCK_VIRTUAL ); + +return 0 ; }static int tsc210x_post_load (void *opaque,int version_id)
hw/intc/apic_common.c +3 -1
@@ -360,7 +360,7 @@ static int apic_pre_load(void *opaque)return 0 ; } -static void apic_dispatch_pre_save (void *opaque) +static int apic_dispatch_pre_save (void *opaque) {APICCommonState *s =APIC_COMMON (opaque);APICCommonClass *info =APIC_COMMON_GET_CLASS (s);
@@ -368,6 +368,8 @@ static void apic_dispatch_pre_save(void *opaque)if (info->pre_save) { info->pre_save (s); } + +return 0 ; }static int apic_dispatch_post_load (void *opaque,int version_id)
hw/intc/arm_gic_common.c +3 -1
@@ -23,7 +23,7 @@#include "gic_internal.h" #include "hw/arm/linux-boot-if.h" -static void gic_pre_save (void *opaque) +static int gic_pre_save (void *opaque) {GICState *s = (GICState *)opaque;ARMGICCommonClass *c =ARM_GIC_COMMON_GET_CLASS (s);
@@ -31,6 +31,8 @@ static void gic_pre_save(void *opaque)if (c->pre_save) { c->pre_save (s); } + +return 0 ; }static int gic_post_load (void *opaque,int version_id)
hw/intc/arm_gicv3_common.c +3 -1
@@ -28,7 +28,7 @@#include "gicv3_internal.h" #include "hw/arm/linux-boot-if.h" -static void gicv3_pre_save (void *opaque) +static int gicv3_pre_save (void *opaque) {GICv3State *s = (GICv3State *)opaque;ARMGICv3CommonClass *c =ARM_GICV3_COMMON_GET_CLASS (s);
@@ -36,6 +36,8 @@ static void gicv3_pre_save(void *opaque)if (c->pre_save) { c->pre_save (s); } + +return 0 ; }static int gicv3_post_load (void *opaque,int version_id)
hw/intc/arm_gicv3_its_common.c +3 -1
@@ -23,7 +23,7 @@#include "hw/intc/arm_gicv3_its_common.h" #include "qemu/log.h" -static void gicv3_its_pre_save (void *opaque) +static int gicv3_its_pre_save (void *opaque) {GICv3ITSState *s = (GICv3ITSState *)opaque;GICv3ITSCommonClass *c =ARM_GICV3_ITS_COMMON_GET_CLASS (s);
@@ -31,6 +31,8 @@ static void gicv3_its_pre_save(void *opaque)if (c->pre_save) { c->pre_save (s); } + +return 0 ; }static int gicv3_its_post_load (void *opaque,int version_id)
hw/intc/i8259_common.c +3 -1
@@ -46,7 +46,7 @@ void pic_reset_common(PICCommonState *s)/* Note: ELCR is not reset */ } -static void pic_dispatch_pre_save (void *opaque) +static int pic_dispatch_pre_save (void *opaque) {PICCommonState *s = opaque;PICCommonClass *info =PIC_COMMON_GET_CLASS (s);
@@ -54,6 +54,8 @@ static void pic_dispatch_pre_save(void *opaque)if (info->pre_save) { info->pre_save (s); } + +return 0 ; }static int pic_dispatch_post_load (void *opaque,int version_id)
hw/intc/ioapic_common.c +3 -1
@@ -102,7 +102,7 @@ void ioapic_reset_common(DeviceState *dev) } } -static void ioapic_dispatch_pre_save (void *opaque) +static int ioapic_dispatch_pre_save (void *opaque) {IOAPICCommonState *s =IOAPIC_COMMON (opaque);IOAPICCommonClass *info =IOAPIC_COMMON_GET_CLASS (s);
@@ -110,6 +110,8 @@ static void ioapic_dispatch_pre_save(void *opaque)if (info->pre_save) { info->pre_save (s); } + +return 0 ; }static int ioapic_dispatch_post_load (void *opaque,int version_id)
hw/intc/s390_flic_kvm.c +4 -2
@@ -420,7 +420,7 @@ typedef struct KVMS390FLICStateMigTmp {uint8_t nimm; } KVMS390FLICStateMigTmp; -static void kvm_flic_ais_pre_save (void *opaque) +static int kvm_flic_ais_pre_save (void *opaque) {KVMS390FLICStateMigTmp *tmp = opaque;KVMS390FLICState *flic = tmp->parent;
@@ -433,11 +433,13 @@ static void kvm_flic_ais_pre_save(void *opaque)if (ioctl (flic->fd,KVM_GET_DEVICE_ATTR , &attr)) {error_report ("Failed to retrieve kvm flic ais states" ); -return ; +return -EINVAL ; } tmp->simm = ais.simm; tmp->nimm = ais.nimm; + +return 0 ; }static int kvm_flic_ais_post_load (void *opaque,int version_id)
hw/intc/xics.c +6 -2
@@ -241,7 +241,7 @@ static void icp_irq(ICSState *ics, int server, int nr, uint8_t priority) } } -static void icp_dispatch_pre_save (void *opaque) +static int icp_dispatch_pre_save (void *opaque) {ICPState *icp = opaque;ICPStateClass *info =ICP_GET_CLASS (icp);
@@ -249,6 +249,8 @@ static void icp_dispatch_pre_save(void *opaque)if (info->pre_save) { info->pre_save (icp); } + +return 0 ; }static int icp_dispatch_post_load (void *opaque,int version_id)
@@ -533,7 +535,7 @@ static void ics_simple_reset(void *dev) } } -static void ics_simple_dispatch_pre_save (void *opaque) +static int ics_simple_dispatch_pre_save (void *opaque) {ICSState *ics = opaque;ICSStateClass *info =ICS_BASE_GET_CLASS (ics);
@@ -541,6 +543,8 @@ static void ics_simple_dispatch_pre_save(void *opaque)if (info->pre_save) { info->pre_save (ics); } + +return 0 ; }static int ics_simple_dispatch_post_load (void *opaque,int version_id)
hw/net/e1000.c +3 -1
@@ -1343,7 +1343,7 @@ static bool is_version_1(void *opaque, int version_id)return version_id ==1 ; } -static void e1000_pre_save (void *opaque) +static int e1000_pre_save (void *opaque) {E1000State *s = opaque;NetClientState *nc =qemu_get_queue (s->nic);
@@ -1361,6 +1361,8 @@ static void e1000_pre_save(void *opaque)if (nc->link_down &&have_autoneg (s)) { s->phy_reg[PHY_STATUS ] |=MII_SR_AUTONEG_COMPLETE ; } + +return 0 ; }static int e1000_post_load (void *opaque,int version_id)
hw/net/e1000e.c +3 -1
@@ -523,13 +523,15 @@ static void e1000e_qdev_reset(DeviceState *dev)e1000e_core_reset (&s->core); } -static void e1000e_pre_save (void *opaque) +static int e1000e_pre_save (void *opaque) {E1000EState *s = opaque;trace_e1000e_cb_pre_save ();e1000e_core_pre_save (&s->core); + +return 0 ; }static int e1000e_post_load (void *opaque,int version_id)
hw/net/rtl8139.c +3 -1
@@ -3162,7 +3162,7 @@ static const VMStateDescription vmstate_rtl8139_hotplug_ready ={ } }; -static void rtl8139_pre_save (void *opaque) +static int rtl8139_pre_save (void *opaque) {RTL8139State * s = opaque;int64_t current_time =qemu_clock_get_ns (QEMU_CLOCK_VIRTUAL );
@@ -3170,6 +3170,8 @@ static void rtl8139_pre_save(void *opaque)/* for migration to older versions */ s->TCTR = (current_time - s->TCTR_base) /PCI_PERIOD ; s->rtl8139_mmio_io_addr_dummy =0 ; + +return 0 ; }static const VMStateDescription vmstate_rtl8139 = {
hw/net/virtio-net.c +12 -4
@@ -1712,7 +1712,7 @@ struct VirtIONetMigTmp {* pointer and count and also validate the count. */ -static void virtio_net_tx_waiting_pre_save (void *opaque) +static int virtio_net_tx_waiting_pre_save (void *opaque) {struct VirtIONetMigTmp *tmp = opaque;
@@ -1721,6 +1721,8 @@ static void virtio_net_tx_waiting_pre_save(void *opaque)if (tmp->parent->curr_queues ==0 ) { tmp->curr_queues_1 =0 ; } + +return 0 ; }static int virtio_net_tx_waiting_pre_load (void *opaque)
@@ -1768,11 +1770,13 @@ static int virtio_net_ufo_post_load(void *opaque, int version_id)return 0 ; } -static void virtio_net_ufo_pre_save (void *opaque) +static int virtio_net_ufo_pre_save (void *opaque) {struct VirtIONetMigTmp *tmp = opaque; tmp->has_ufo = tmp->parent->has_ufo; + +return 0 ; }static const VMStateDescription vmstate_virtio_net_has_ufo = {
@@ -1800,11 +1804,13 @@ static int virtio_net_vnet_post_load(void *opaque, int version_id)return 0 ; } -static void virtio_net_vnet_pre_save (void *opaque) +static int virtio_net_vnet_pre_save (void *opaque) {struct VirtIONetMigTmp *tmp = opaque; tmp->has_vnet_hdr = tmp->parent->has_vnet_hdr; + +return 0 ; }static const VMStateDescription vmstate_virtio_net_has_vnet = {
@@ -2079,13 +2085,15 @@ static void virtio_net_instance_init(Object *obj) DEVICE(n), NULL); } -static void virtio_net_pre_save (void *opaque) +static int virtio_net_pre_save (void *opaque) {VirtIONet *n = opaque;/* At this point, backend must be stopped, otherwise * it might keep writing to memory. */ assert (!n->vhost_started); + +return 0 ; }static const VMStateDescription vmstate_virtio_net = {
hw/net/vmxnet3.c +3 -1
@@ -2388,11 +2388,13 @@ static int vmxnet3_mcast_list_pre_load(void *opaque) } -static void vmxnet3_pre_save (void *opaque) +static int vmxnet3_pre_save (void *opaque) {VMXNET3State *s = opaque; s->mcast_list_buff_size = s->mcast_list_len *sizeof (MACAddr); + +return 0 ; }static const VMStateDescription vmxstate_vmxnet3_mcast_list = {
hw/pci-host/piix.c +3 -1
@@ -579,7 +579,7 @@ static int piix3_post_load(void *opaque, int version_id)return 0 ; } -static void piix3_pre_save (void *opaque) +static int piix3_pre_save (void *opaque) {int i;PIIX3State *piix3 = opaque;
@@ -588,6 +588,8 @@ static void piix3_pre_save(void *opaque) piix3->pci_irq_levels_vmstate[i] =pci_bus_get_irq_level (piix3->dev.bus, i); } + +return 0 ; }static bool piix3_rcr_needed (void *opaque)
hw/ppc/ppc.c +3 -1
@@ -937,11 +937,13 @@ void cpu_ppc_clock_vm_state_change(void *opaque, int running,* final pages of memory (which happens between vm_stop() * and pre_save()) takes max_downtime. */ -static void timebase_pre_save (void *opaque) +static int timebase_pre_save (void *opaque) {PPCTimebase *tb = opaque;timebase_save (tb); + +return 0 ; }const VMStateDescription vmstate_ppc_timebase = {
hw/ppc/spapr_iommu.c +3 -1
@@ -140,7 +140,7 @@ static IOMMUTLBEntry spapr_tce_translate_iommu(IOMMUMemoryRegion *iommu,return ret; } -static void spapr_tce_table_pre_save (void *opaque) +static int spapr_tce_table_pre_save (void *opaque) {sPAPRTCETable *tcet =SPAPR_TCE_TABLE (opaque);
@@ -149,6 +149,8 @@ static void spapr_tce_table_pre_save(void *opaque)trace_spapr_iommu_pre_save (tcet->liobn, tcet->mig_nb_table, tcet->bus_offset, tcet->page_shift); + +return 0 ; }static uint64_t spapr_tce_get_min_page_size (IOMMUMemoryRegion *iommu)
hw/ppc/spapr_pci.c +4 -2
@@ -1860,7 +1860,7 @@ static const VMStateDescription vmstate_spapr_pci_msi = { }, }; -static void spapr_pci_pre_save (void *opaque) +static int spapr_pci_pre_save (void *opaque) {sPAPRPHBState *sphb = opaque;GHashTableIter iter;
@@ -1885,7 +1885,7 @@ static void spapr_pci_pre_save(void *opaque) sphb->msi_devs =NULL ; sphb->msi_devs_num =g_hash_table_size (sphb->msi);if (!sphb->msi_devs_num) { -return ; +return 0 ; } sphb->msi_devs =g_malloc (sphb->msi_devs_num *sizeof (spapr_pci_msi_mig));
@@ -1894,6 +1894,8 @@ static void spapr_pci_pre_save(void *opaque) sphb->msi_devs[i].key = *(uint32_t *) key; sphb->msi_devs[i].value = *(spapr_pci_msi *) value; } + +return 0 ; }static int spapr_pci_post_load (void *opaque,int version_id)
hw/s390x/css.c +7 -3
@@ -196,7 +196,7 @@ static const VMStateDescription vmstate_schdev_orb = { };static int subch_dev_post_load (void *opaque,int version_id); -static void subch_dev_pre_save (void *opaque); +static int 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"
@@ -249,7 +249,7 @@ static int post_load_ind_addr(void *opaque, int version_id)return 0 ; } -static void pre_save_ind_addr (void *opaque) +static int pre_save_ind_addr (void *opaque) {IndAddrPtrTmp *ptmp = opaque;IndAddr *ind_addr = *(ptmp->parent);
@@ -261,6 +261,8 @@ static void pre_save_ind_addr(void *opaque) ptmp->len =0 ; ptmp->addr =0L ; } + +return 0 ; }const VMStateDescription vmstate_ind_addr_tmp = {
@@ -358,12 +360,14 @@ static ChannelSubSys channel_subsys = { QTAILQ_HEAD_INITIALIZER(channel_subsys.indicator_addresses), }; -static void subch_dev_pre_save (void *opaque) +static int subch_dev_pre_save (void *opaque) {SubchDev *s = opaque;/* Prepare remote_schid for save */ s->migrated_schid = s->schid; + +return 0 ; }static int subch_dev_post_load (void *opaque,int version_id)
hw/s390x/virtio-ccw.c +3 -1
@@ -62,13 +62,15 @@ typedef struct VirtioCcwDeviceTmp {uint16_t config_vector; } VirtioCcwDeviceTmp; -static void virtio_ccw_dev_tmp_pre_save (void *opaque) +static int virtio_ccw_dev_tmp_pre_save (void *opaque) {VirtioCcwDeviceTmp *tmp = opaque;VirtioCcwDevice *dev = tmp->parent;VirtIODevice *vdev =virtio_bus_get_device (&dev->bus); tmp->config_vector = vdev->config_vector; + +return 0 ; }static int virtio_ccw_dev_tmp_post_load (void *opaque,int version_id)
hw/scsi/lsi53c895a.c +3 -1
@@ -2083,7 +2083,7 @@ static void lsi_scsi_reset(DeviceState *dev)lsi_soft_reset (s); } -static void lsi_pre_save (void *opaque) +static int lsi_pre_save (void *opaque) {LSIState *s = opaque;
@@ -2092,6 +2092,8 @@ static void lsi_pre_save(void *opaque)assert (s->current->dma_len ==0 ); }assert (QTAILQ_EMPTY (&s->queue)); + +return 0 ; }static const VMStateDescription vmstate_lsi_scsi = {
hw/scsi/vmw_pvscsi.c +3 -1
@@ -1167,7 +1167,7 @@ pvscsi_reset(DeviceState *dev)pvscsi_reset_adapter (s); } -static void +static int pvscsi_pre_save (void *opaque) {PVSCSIState *s = (PVSCSIState *) opaque;
@@ -1176,6 +1176,8 @@ pvscsi_pre_save(void *opaque)assert (QTAILQ_EMPTY (&s->pending_queue));assert (QTAILQ_EMPTY (&s->completion_queue)); + +return 0 ; } static int
hw/timer/cadence_ttc.c +3 -1
@@ -421,9 +421,11 @@ static void cadence_ttc_init(Object *obj)sysbus_init_mmio (SYS_BUS_DEVICE (obj), &s->iomem); } -static void cadence_timer_pre_save (void *opaque) +static int cadence_timer_pre_save (void *opaque) {cadence_timer_sync ((CadenceTimerState *)opaque); + +return 0 ; }static int cadence_timer_post_load (void *opaque,int version_id)
hw/timer/hpet.c +3 -1
@@ -216,12 +216,14 @@ static void update_irq(struct HPETTimer *timer, int set) } } -static void hpet_pre_save (void *opaque) +static int hpet_pre_save (void *opaque) {HPETState *s = opaque;/* save current counter value */ s->hpet_counter =hpet_get_ticks (s); + +return 0 ; }static int hpet_pre_load (void *opaque)
hw/timer/i8254_common.c +3 -1
@@ -237,7 +237,7 @@ static int pit_load_old(QEMUFile *f, void *opaque, int version_id)return 0 ; } -static void pit_dispatch_pre_save (void *opaque) +static int pit_dispatch_pre_save (void *opaque) {PITCommonState *s = opaque;PITCommonClass *c =PIT_COMMON_GET_CLASS (s);
@@ -245,6 +245,8 @@ static void pit_dispatch_pre_save(void *opaque)if (c->pre_save) { c->pre_save (s); } + +return 0 ; }static int pit_dispatch_post_load (void *opaque,int version_id)
hw/timer/mc146818rtc.c +3 -1
@@ -795,11 +795,13 @@ static void rtc_set_date_from_host(ISADevice *dev)rtc_set_cmos (s, &tm); } -static void rtc_pre_save (void *opaque) +static int rtc_pre_save (void *opaque) {RTCState *s = opaque;rtc_update_time (s); + +return 0 ; }static int rtc_post_load (void *opaque,int version_id)
hw/timer/pl031.c +3 -1
@@ -211,7 +211,7 @@ static void pl031_init(Object *obj) s->timer =timer_new_ns (rtc_clock, pl031_interrupt, s); } -static void pl031_pre_save (void *opaque) +static int pl031_pre_save (void *opaque) {PL031State *s = opaque;
@@ -219,6 +219,8 @@ static void pl031_pre_save(void *opaque)* store the base time relative to the QEMU_CLOCK_VIRTUAL for backwards-compatibility. */ int64_t delta =qemu_clock_get_ns (rtc_clock) -qemu_clock_get_ns (QEMU_CLOCK_VIRTUAL ); s->tick_offset_vmstate = s->tick_offset + delta /NANOSECONDS_PER_SECOND ; + +return 0 ; }static int pl031_post_load (void *opaque,int version_id)
hw/timer/twl92230.c +3 -1
@@ -791,11 +791,13 @@ static const VMStateDescription vmstate_menelaus_tm = { } }; -static void menelaus_pre_save (void *opaque) +static int menelaus_pre_save (void *opaque) {MenelausState *s = opaque;/* Should be <= 1000 */ s->rtc_next_vmstate = s->rtc.next -qemu_clock_get_ms (rtc_clock); + +return 0 ; }static int menelaus_post_load (void *opaque,int version_id)
hw/usb/dev-smartcard-reader.c +3 -1
@@ -1374,7 +1374,7 @@ static int ccid_post_load(void *opaque, int version_id)return 0 ; } -static void ccid_pre_save (void *opaque) +static int ccid_pre_save (void *opaque) {USBCCIDState *s = opaque;
@@ -1386,6 +1386,8 @@ static void ccid_pre_save(void *opaque)*/ s->migration_state =MIGRATION_MIGRATED ; } + +return 0 ; }static VMStateDescription bulk_in_vmstate = {
hw/usb/hcd-ehci.c +3 -1
@@ -2380,7 +2380,7 @@ static USBBusOps ehci_bus_ops_standalone = { .wakeup_endpoint = ehci_wakeup_endpoint, }; -static void usb_ehci_pre_save (void *opaque) +static int usb_ehci_pre_save (void *opaque) {EHCIState *ehci = opaque;uint32_t new_frindex;
@@ -2389,6 +2389,8 @@ static void usb_ehci_pre_save(void *opaque) new_frindex = ehci->frindex & ~7 ; ehci->last_run_ns -= (ehci->frindex - new_frindex) *UFRAME_TIMER_NS ; ehci->frindex = new_frindex; + +return 0 ; }static int usb_ehci_post_load (void *opaque,int version_id)
hw/usb/redirect.c +3 -1
@@ -2111,11 +2111,13 @@ static void usbredir_buffered_bulk_packet(void *priv, uint64_t id,* Migration code */ -static void usbredir_pre_save (void *priv) +static int usbredir_pre_save (void *priv) {USBRedirDevice *dev = priv;usbredir_fill_already_in_flight (dev); + +return 0 ; }static int usbredir_post_load (void *priv,int version_id)
hw/virtio/vhost-vsock.c +3 -1
@@ -254,13 +254,15 @@ static void vhost_vsock_post_load_timer_cb(void *opaque)vhost_vsock_send_transport_reset (vsock); } -static void vhost_vsock_pre_save (void *opaque) +static int vhost_vsock_pre_save (void *opaque) {VHostVSock *vsock = opaque;/* At this point, backend must be stopped, otherwise * it might keep writing to memory. */ assert (!vsock->vhost_dev.started); + +return 0 ; }static int vhost_vsock_post_load (void *opaque,int version_id)
include/migration/vmstate.h +1 -1
@@ -179,7 +179,7 @@ struct VMStateDescription {LoadStateHandler *load_state_old;int (*pre_load)(void *opaque);int (*post_load)(void *opaque,int version_id); -void (*pre_save)(void *opaque); +int (*pre_save)(void *opaque);bool (*needed)(void *opaque);VMStateField *fields;const VMStateDescription **subsections;
migration/colo-comm.c +3 -1
@@ -34,11 +34,13 @@ COLOMode get_colo_mode(void) } } -static void colo_info_pre_save (void *opaque) +static int colo_info_pre_save (void *opaque) {COLOInfo *s = opaque; s->colo_requested =migrate_colo_enabled (); + +return 0 ; }static bool colo_info_need (void *opaque)
migration/global_state.c +3 -1
@@ -101,12 +101,14 @@ static int global_state_post_load(void *opaque, int version_id)return 0 ; } -static void global_state_pre_save (void *opaque) +static int global_state_pre_save (void *opaque) {GlobalState *s = opaque;trace_migrate_global_state_pre_save ((char *)s->runstate); s->size =strlen ((char *)s->runstate) +1 ; + +return 0 ; }static const VMStateDescription vmstate_globalstate = {
migration/savevm.c +3 -1
@@ -295,7 +295,7 @@ static SaveState savevm_state = { .global_section_id = 0, }; -static void configuration_pre_save (void *opaque) +static int configuration_pre_save (void *opaque) {SaveState *state = opaque;const char *current_name =MACHINE_GET_CLASS (current_machine)->name;
@@ -303,6 +303,8 @@ static void configuration_pre_save(void *opaque) state->len =strlen (current_name); state->name = current_name; state->target_page_bits =qemu_target_page_bits (); + +return 0 ; }static int configuration_pre_load (void *opaque)
replay/replay-snapshot.c +3 -1
@@ -21,10 +21,12 @@#include "migration/vmstate.h" #include "migration/snapshot.h" -static void replay_pre_save (void *opaque) +static int replay_pre_save (void *opaque) {ReplayState *state = opaque; state->file_offset =ftell (replay_file); + +return 0 ; }static int replay_post_load (void *opaque,int version_id)
slirp/slirp.c +6 -2
@@ -1203,11 +1203,13 @@ struct sbuf_tmp {uint32_t roff, woff; }; -static void sbuf_tmp_pre_save (void *opaque) +static int sbuf_tmp_pre_save (void *opaque) {struct sbuf_tmp *tmp = opaque; tmp->woff = tmp->parent->sb_wptr - tmp->parent->sb_data; tmp->roff = tmp->parent->sb_rptr - tmp->parent->sb_data; + +return 0 ; }static int sbuf_tmp_post_load (void *opaque,int version)
@@ -1303,7 +1305,7 @@ typedef struct SS_FamilyTmpStruct {#define SS_FAMILY_MIG_IPV6 10/* Linux */ #define SS_FAMILY_MIG_OTHER 0xffff -static void ss_family_pre_save (void *opaque) +static int ss_family_pre_save (void *opaque) {SS_FamilyTmpStruct *tss = opaque;
@@ -1314,6 +1316,8 @@ static void ss_family_pre_save(void *opaque) }else if (tss->parent->ss.ss_family ==AF_INET6 ) { tss->portable_family =SS_FAMILY_MIG_IPV6 ; } + +return 0 ; }static int ss_family_post_load (void *opaque,int version_id)
target/arm/machine.c +3 -1
@@ -394,7 +394,7 @@ static const VMStateInfo vmstate_powered_off = { .put = put_power, }; -static void cpu_pre_save (void *opaque) +static int cpu_pre_save (void *opaque) {ARMCPU *cpu = opaque;
@@ -415,6 +415,8 @@ static void cpu_pre_save(void *opaque) cpu->cpreg_array_len * sizeof(uint64_t));memcpy (cpu->cpreg_vmstate_values, cpu->cpreg_values, cpu->cpreg_array_len *sizeof (uint64_t )); + +return 0 ; }static int cpu_post_load (void *opaque,int version_id)
target/i386/machine.c +5 -2
@@ -160,13 +160,15 @@ static floatx80 cpu_set_fp80(uint64_t mant, uint16_t upper)return temp.d; } -static void fpreg_pre_save (void *opaque) +static int fpreg_pre_save (void *opaque) {x86_FPReg_tmp *tmp = opaque;/* we save the real CPU data (in case of MMX usage only 'mant' contains the MMX register */ cpu_get_fp80 (&tmp->tmp_mant, &tmp->tmp_exp, tmp->parent->d); + +return 0 ; }static int fpreg_post_load (void *opaque,int version)
@@ -196,7 +198,7 @@ static const VMStateDescription vmstate_fpreg = { } }; -static void cpu_pre_save (void *opaque) +static int cpu_pre_save (void *opaque) {X86CPU *cpu = opaque;CPUX86State *env = &cpu->env;
@@ -228,6 +230,7 @@ static void cpu_pre_save(void *opaque) env->segs[R_SS ].flags &= ~(env->segs[R_SS ].flags &DESC_DPL_MASK ); } +return 0 ; }static int cpu_post_load (void *opaque,int version_id)
target/ppc/machine.c +3 -1
@@ -146,7 +146,7 @@ static bool cpu_pre_2_8_migration(void *opaque, int version_id)return cpu->pre_2_8_migration; } -static void cpu_pre_save (void *opaque) +static int cpu_pre_save (void *opaque) {PowerPCCPU *cpu = opaque;CPUPPCState *env = &cpu->env;
@@ -195,6 +195,8 @@ static void cpu_pre_save(void *opaque) cpu->mig_insns_flags2 = env->insns_flags2 & insns_compat_mask2; cpu->mig_nb_BATs = env->nb_BATs; } + +return 0 ; }/*
target/s390x/machine.c +3 -1
@@ -37,13 +37,15 @@ static int cpu_post_load(void *opaque, int version_id)return 0 ; } -static void cpu_pre_save (void *opaque) +static int cpu_pre_save (void *opaque) {S390CPU *cpu = opaque;if (kvm_enabled ()) {kvm_s390_vcpu_interrupt_pre_save (cpu); } + +return 0 ; }static inline bool fpu_needed (void *opaque)
target/sparc/machine.c +3 -1
@@ -88,7 +88,7 @@ static const VMStateInfo vmstate_psr = { .put = put_psr, }; -static void cpu_pre_save (void *opaque) +static int cpu_pre_save (void *opaque) {SPARCCPU *cpu = opaque;CPUSPARCState *env = &cpu->env;
@@ -97,6 +97,8 @@ static void cpu_pre_save(void *opaque)* window as the outs of the first window */ cpu_set_cwp (env, env->cwp); + +return 0 ; }/* 32-bit SPARC retains migration compatibility with older versions
tests/test-vmstate.c +3 -1
@@ -765,11 +765,13 @@ typedef struct TmpTestStruct {int64_t diff; } TmpTestStruct; -static void tmp_child_pre_save (void *opaque) +static int tmp_child_pre_save (void *opaque) {struct TmpTestStruct *tts = opaque; tts->diff = tts->parent->b - tts->parent->a; + +return 0 ; }static int tmp_child_post_load (void *opaque,int version_id)
Signature · 1 line
2.13.5
Re: [Qemu-devel] [PATCH 1/5] migration: pre_save return int
Quoted 8 lines · On Tue, 19 Sep 2017 19:00:34 +0100 "Dr. David Alan Gilbert (git)"…
From: "Dr. David Alan Gilbert" <[email protected]>
Modify the
pre_savemethod on VMStateDescription to return an int rather than void so that it potentially can fail.Changed zillions of devices to make them return 0; the only case I've made it return non-0 is
hw/intc/s390_flic_kvm.cthat already had an error_report/return case.
Never thought that this device would be at the bleeding edge ;)
Quoted 3 lines
Note: If you add an error exit in your
pre_saveyou must emit anerror_reportto say why.
Would it make sense to relay an error object? For example, cpu_pre_save() in target/s390x/machine.c calls kvm_s390_vcpu_interrupt_pre_save() which already does an error report. If we relay that error instead, we would avoid saying "oops, this didn't work" several times with decreasing amount of information.
On the other hand, that change would be more invasive.
Quoted 10 lines
Signed-off-by Dr. David Alan Gilbert <[email protected]>---
hw/intc/s390_flic_kvm.c | 6 ++++--hw/s390x/css.c | 10 +++++++--- hw/s390x/virtio-ccw.c | 4 +++-target/s390x/machine.c | 4 +++-
That said, the changes in s390-related code look fine.
Re: [Qemu-devel] [PATCH 1/5] migration: pre_save return int
Quoted 5 lines · * Cornelia Huck ([email protected]) wrote:
Quoted 2 lines · On Tue, 19 Sep 2017 19:00:34 +0100 "Dr. David Alan Gilbert (git)"…
From: "Dr. David Alan Gilbert" <[email protected]>
Quoted 8 lines
Quoted 6 lines
Modify the
pre_savemethod on VMStateDescription to return an int rather than void so that it potentially can fail.Changed zillions of devices to make them return 0; the only case I've made it return non-0 is
hw/intc/s390_flic_kvm.cthat already had an error_report/return case.Never thought that this device would be at the bleeding edge ;)
It's the one case that had bothered to do a proper error.
Quoted 11 lines
Quoted 3 lines
Note: If you add an error exit in your
pre_saveyou must emit anerror_reportto say why.Would it make sense to relay an error object? For example,
cpu_pre_save()intarget/s390x/machine.ccallskvm_s390_vcpu_interrupt_pre_save()which already does an error report. If we relay that error instead, we would avoid saying "oops, this didn't work" several times with decreasing amount of information.On the other hand, that change would be more invasive.
Right, and it's very very verbose.
Quoted 5 lines
Quoted 4 lines
Signed-off-by Dr. David Alan Gilbert <[email protected]>---
Quoted 8 lines
Quoted 6 lines
hw/intc/s390_flic_kvm.c | 6 ++++--hw/s390x/css.c | 10 +++++++--- hw/s390x/virtio-ccw.c | 4 +++-target/s390x/machine.c | 4 +++-That said, the changes in s390-related code look fine.
Thanks.
Dave
--
Dr. David Alan Gilbert / [email protected] / Manchester, UK
PATCH 2/5 migration: check pre_save return in vmstate_save_state
In reply to Dr. David Alan Gilbert (git)From: "Dr. David Alan Gilbert" <[email protected]>
Check the return value of pre_save state and fail vmstate_save_state if the pre_save failed.
--- include/migration/vmstate.h | 4 ++-- migration/trace-events | 1 + migration/vmstate.c | 18 +++++++++++++++--- 3 files changed, 18 insertions(+), 5 deletions(-)
include/migration/vmstate.h +2 -2
@@ -994,8 +994,8 @@ extern const VMStateInfo vmstate_info_qtailq;int vmstate_load_state (QEMUFile *f,const VMStateDescription *vmsd,void *opaque,int version_id); -void vmstate_save_state (QEMUFile *f,const VMStateDescription *vmsd, -void *opaque,QJSON *vmdesc); +int vmstate_save_state (QEMUFile *f,const VMStateDescription *vmsd, +void *opaque,QJSON *vmdesc);bool vmstate_save_needed (const VMStateDescription *vmsd,void *opaque);
migration/trace-events +1 -0
@@ -40,6 +40,7 @@ savevm_state_iterate(void) "" savevm_state_cleanup(void) "" savevm_state_complete_precopy(void) "" vmstate_save(const char *idstr, const char *vmsd_name) "%s, %s" +vmstate_save_state_pre_save_res(const char *name, int res) "%s/%d" vmstate_save_state_loop(const char *name, const char *field, int n_elems) "%s/%s[%d]" vmstate_save_state_top(const char *idstr) "%s" vmstate_subsection_save_loop(const char *name, const char *sub) "%s/%s"
migration/vmstate.c +15 -3
@@ -308,15 +308,21 @@ bool vmstate_save_needed(const VMStateDescription *vmsd, void *opaque) } -void vmstate_save_state (QEMUFile *f,const VMStateDescription *vmsd, +int vmstate_save_state (QEMUFile *f,const VMStateDescription *vmsd,void *opaque,QJSON *vmdesc) { +int ret =0 ;VMStateField *field = vmsd->fields;trace_vmstate_save_state_top (vmsd->name);if (vmsd->pre_save) { - vmsd->pre_save (opaque); + ret = vmsd->pre_save (opaque); +trace_vmstate_save_state_pre_save_res (vmsd->name, ret); +if (ret) { +error_report ("pre-save failed: %s" , vmsd->name); +return ret; + } }if (vmdesc) {
@@ -353,7 +359,11 @@ void vmstate_save_state(QEMUFile *f, const VMStateDescription *vmsd,assert (field->flags &VMS_ARRAY_OF_POINTER ); vmstate_info_nullptr.put (f, curr_elem, size,NULL ,NULL ); }else if (field->flags &VMS_STRUCT ) { -vmstate_save_state (f, field->vmsd, curr_elem, vmdesc_loop); +int ret =vmstate_save_state (f, field->vmsd, curr_elem, + vmdesc_loop); +if (ret) { +return ret; + } }else { field->info->put (f, curr_elem, size, field, vmdesc_loop); }
@@ -381,6 +391,8 @@ void vmstate_save_state(QEMUFile *f, const VMStateDescription *vmsd, }vmstate_subsection_save (f, vmsd, opaque, vmdesc); + +return 0 ; } static const VMStateDescription *
Signature · 1 line
2.13.5
PATCH 3/5 migration: wire vmstate_save_state errors up to vmstate_subsection_save
In reply to Dr. David Alan Gilbert (git)From: "Dr. David Alan Gilbert" <[email protected]>
Route the errors from vmstate_subsection_save up through vmstate_subsection_save (and back down, all rather recursive).
--- migration/vmstate.c | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-)
migration/vmstate.c +12 -8
@@ -21,8 +21,8 @@#include "trace.h" #include "qjson.h" -static void vmstate_subsection_save (QEMUFile *f,const VMStateDescription *vmsd, -void *opaque,QJSON *vmdesc); +static int vmstate_subsection_save (QEMUFile *f,const VMStateDescription *vmsd, +void *opaque,QJSON *vmdesc);static int vmstate_subsection_load (QEMUFile *f,const VMStateDescription *vmsd,void *opaque);
@@ -390,9 +390,7 @@ int vmstate_save_state(QEMUFile *f, const VMStateDescription *vmsd,json_end_array (vmdesc); } -vmstate_subsection_save (f, vmsd, opaque, vmdesc); - -return 0 ; +return vmstate_subsection_save (f, vmsd, opaque, vmdesc); } static const VMStateDescription *
@@ -458,11 +456,12 @@ static int vmstate_subsection_load(QEMUFile *f, const VMStateDescription *vmsd,return 0 ; } -static void vmstate_subsection_save (QEMUFile *f,const VMStateDescription *vmsd, -void *opaque,QJSON *vmdesc) +static int vmstate_subsection_save (QEMUFile *f,const VMStateDescription *vmsd, +void *opaque,QJSON *vmdesc) {const VMStateDescription **sub = vmsd->subsections;bool subsection_found = false; +int ret =0 ;trace_vmstate_subsection_save_top (vmsd->name);while (sub && *sub && (*sub)->needed) {
@@ -486,7 +485,10 @@ static void vmstate_subsection_save(QEMUFile *f, const VMStateDescription *vmsd,qemu_put_byte (f, len);qemu_put_buffer (f, (uint8_t *)vmsdsub->name, len);qemu_put_be32 (f, vmsdsub->version_id); -vmstate_save_state (f, vmsdsub, opaque, vmdesc); + ret =vmstate_save_state (f, vmsdsub, opaque, vmdesc); +if (ret) { +return ret; + }if (vmdesc) {json_end_object (vmdesc);
@@ -498,4 +500,6 @@ static void vmstate_subsection_save(QEMUFile *f, const VMStateDescription *vmsd,if (vmdesc && subsection_found) {json_end_array (vmdesc); } + +return ret; }
Signature · 1 line
2.13.5
PATCH 4/5 migration: Route errors up through vmstate_save
In reply to Dr. David Alan Gilbert (git)From: "Dr. David Alan Gilbert" <[email protected]>
Route the errors from vsmtate_save_state back up through vmstate_save and out to the normal device state path. That's the normal error path done.
--- migration/savevm.c | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-)
migration/savevm.c +14 -5
@@ -751,14 +751,14 @@ static void vmstate_save_old_style(QEMUFile *f, SaveStateEntry *se, QJSON *vmdes } } -static void vmstate_save (QEMUFile *f,SaveStateEntry *se,QJSON *vmdesc) +static int vmstate_save (QEMUFile *f,SaveStateEntry *se,QJSON *vmdesc) {trace_vmstate_save (se->idstr, se->vmsd ? se->vmsd->name :"(old)" );if (!se->vmsd) {vmstate_save_old_style (f, se, vmdesc); -return ; +return 0 ; } -vmstate_save_state (f, se->vmsd, se->opaque, vmdesc); +return vmstate_save_state (f, se->vmsd, se->opaque, vmdesc); }/*
@@ -1147,7 +1147,11 @@ int qemu_savevm_state_complete_precopy(QEMUFile *f, bool iterable_only,json_prop_int (vmdesc,"instance_id" , se->instance_id);save_section_header (f, se,QEMU_VM_SECTION_FULL ); -vmstate_save (f, se, vmdesc); + ret =vmstate_save (f, se, vmdesc); +if (ret) { +qemu_file_set_error (f, ret); +return ret; + }trace_savevm_section_end (se->idstr, se->section_id,0 );save_section_footer (f, se);
@@ -1289,6 +1293,8 @@ static int qemu_save_device_state(QEMUFile *f)cpu_synchronize_all_states (); QTAILQ_FOREACH(se, &savevm_state.handlers, entry) { +int ret; +if (se->is_ram) {continue ; }
@@ -1301,7 +1307,10 @@ static int qemu_save_device_state(QEMUFile *f)save_section_header (f, se,QEMU_VM_SECTION_FULL ); -vmstate_save (f, se,NULL ); + ret =vmstate_save (f, se,NULL ); +if (ret) { +return ret; + }save_section_footer (f, se); }
Signature · 1 line
2.13.5
PATCH 5/5 migration: Route more error paths
In reply to Dr. David Alan Gilbert (git)From: "Dr. David Alan Gilbert" <[email protected]>
vmstate_save is called in a few places, and vmstate_save_state is called in lots of places.
Route error returns from the easier cases back up; there are lots of more complex cases where there own error paths need fixing.
--- hw/display/virtio-gpu.c | 4 +--- hw/virtio/virtio.c | 13 +++++++------ include/hw/virtio/virtio.h | 2 +- migration/vmstate-types.c | 11 ++++++++--- tests/test-vmstate.c | 6 +++--- 5 files changed, 20 insertions(+), 16 deletions(-)
hw/display/virtio-gpu.c +1 -3
@@ -996,9 +996,7 @@ static int virtio_gpu_save(QEMUFile *f, void *opaque, size_t size, }qemu_put_be32 (f,0 );/* end of list */ -vmstate_save_state (f, &vmstate_virtio_gpu_scanouts, g,NULL ); - -return 0 ; +return vmstate_save_state (f, &vmstate_virtio_gpu_scanouts, g,NULL ); } static int virtio_gpu_load(QEMUFile *f, void *opaque, size_t size,
hw/virtio/virtio.c +7 -6
@@ -1899,7 +1899,7 @@ static const VMStateDescription vmstate_virtio = { } }; -void virtio_save (VirtIODevice *vdev,QEMUFile *f) +int virtio_save (VirtIODevice *vdev,QEMUFile *f) {BusState *qbus =qdev_get_parent_bus (DEVICE (vdev));VirtioBusClass *k =VIRTIO_BUS_GET_CLASS (qbus);
@@ -1949,20 +1949,21 @@ void virtio_save(VirtIODevice *vdev, QEMUFile *f) }if (vdc->vmsd) { -vmstate_save_state (f, vdc->vmsd, vdev,NULL ); +int ret =vmstate_save_state (f, vdc->vmsd, vdev,NULL ); +if (ret) { +return ret; + } }/* Subsections */ -vmstate_save_state (f, &vmstate_virtio, vdev,NULL ); +return vmstate_save_state (f, &vmstate_virtio, vdev,NULL ); }/* A wrapper for use as a VMState .put function */ static int virtio_device_put (QEMUFile *f,void *opaque,size_t size,VMStateField *field,QJSON *vmdesc) { -virtio_save (VIRTIO_DEVICE (opaque), f); - -return 0 ; +return virtio_save (VIRTIO_DEVICE (opaque), f); }/* A wrapper for use as a VMState .get function */
include/hw/virtio/virtio.h +1 -1
@@ -188,7 +188,7 @@ void virtqueue_get_avail_bytes(VirtQueue *vq, unsigned int *in_bytes,void virtio_notify_irqfd (VirtIODevice *vdev,VirtQueue *vq);void virtio_notify (VirtIODevice *vdev,VirtQueue *vq); -void virtio_save (VirtIODevice *vdev,QEMUFile *f); +int virtio_save (VirtIODevice *vdev,QEMUFile *f);extern const VMStateInfo virtio_vmstate_info;
migration/vmstate-types.c +8 -3
@@ -550,13 +550,14 @@ static int put_tmp(QEMUFile *f, void *pv, size_t size, VMStateField *field, {const VMStateDescription *vmsd = field->vmsd;void *tmp =g_malloc (size); +int ret;/* Writes the parent field which is at the start of the tmp */ *(void **)tmp = pv; -vmstate_save_state (f, vmsd, tmp, vmdesc); + ret =vmstate_save_state (f, vmsd, tmp, vmdesc);g_free (tmp); -return 0 ; +return ret; }const VMStateInfo vmstate_info_tmp = {
@@ -657,12 +658,16 @@ static int put_qtailq(QEMUFile *f, void *pv, size_t unused_size,/* offset of the QTAILQ entry in a QTAILQ element*/ size_t entry_offset = field->start;void *elm; +int ret;trace_put_qtailq (vmsd->name, vmsd->version_id); QTAILQ_RAW_FOREACH(elm, pv, entry_offset) {qemu_put_byte (f, true); -vmstate_save_state (f, vmsd, elm, vmdesc); + ret =vmstate_save_state (f, vmsd, elm, vmdesc); +if (ret) { +return ret; + } }qemu_put_byte (f, false);
tests/test-vmstate.c +3 -3
@@ -70,7 +70,7 @@ static void save_vmstate(const VMStateDescription *desc, void *obj)QEMUFile *f =open_test_file (true);/* Save file with vmstate */ -vmstate_save_state (f, desc, obj,NULL ); +g_assert (!vmstate_save_state (f, desc, obj,NULL ));qemu_put_byte (f,QEMU_VM_EOF );g_assert (!qemu_file_get_error (f));qemu_fclose (f);
@@ -381,7 +381,7 @@ static void test_save_noskip(void)QEMUFile *fsave =open_test_file (true);TestStruct obj = { .a =1 , .b =2 , .c =3 , .d =4 , .e =5 , .f =6 , .skip_c_e = false }; -vmstate_save_state (fsave, &vmstate_skipping, &obj,NULL ); +g_assert (!vmstate_save_state (fsave, &vmstate_skipping, &obj,NULL ));g_assert (!qemu_file_get_error (fsave));uint8_t expected[] = {
@@ -402,7 +402,7 @@ static void test_save_skip(void)QEMUFile *fsave =open_test_file (true);TestStruct obj = { .a =1 , .b =2 , .c =3 , .d =4 , .e =5 , .f =6 , .skip_c_e = true }; -vmstate_save_state (fsave, &vmstate_skipping, &obj,NULL ); +g_assert (!vmstate_save_state (fsave, &vmstate_skipping, &obj,NULL ));g_assert (!qemu_file_get_error (fsave));uint8_t expected[] = {
Signature · 1 line
2.13.5
Re: [Qemu-devel] [PATCH 5/5] migration: Route more error paths
Quoted 9 lines · Dr. David Alan Gilbert (git)
From: "Dr. David Alan Gilbert" <[email protected]>
vmstate_saveis called in a few places, andvmstate_save_stateis called in lots of places.Route error returns from the easier cases back up; there are lots of more complex cases where there own error paths need fixing.
Signed-off-by Dr. David Alan Gilbert <[email protected]>
IIUC we are touching up the put()s hooks. Then do we need this as well below?
-----
migration/vmstate.c +4 -1
@@ -355,7 +355,10 @@ void vmstate_save_state(QEMUFile *f, const VMStateDescription *vmsd, }else if (field->flags &VMS_STRUCT ) {vmstate_save_state (f, field->vmsd, curr_elem, vmdesc_loop); }else { - field->info->put (f, curr_elem, size, field, vmdesc_loop); + ret = field->info->put (f, curr_elem, size, field, vmdesc_loop); +if (ret) { +return ret; + } } written_bytes =qemu_ftell_fast (f) - old_offset;
-----
Or these errors won't really stop the migration?
Quoted 149 lines
--- hw/display/virtio-gpu.c | 4 +--- hw/virtio/virtio.c | 13 +++++++------ include/hw/virtio/virtio.h | 2 +- migration/vmstate-types.c | 11 ++++++++--- tests/test-vmstate.c | 6 +++--- 5 files changed, 20 insertions(+), 16 deletions(-)hw/display/virtio-gpu.c +1
-3@@ -996,9 +996,7 @@ static int virtio_gpu_save(QEMUFile *f, void *opaque, size_t size, }qemu_put_be32 (f,0 );/* end of list */ -vmstate_save_state (f, &vmstate_virtio_gpu_scanouts, g,NULL ); - -return 0 ; +return vmstate_save_state (f, &vmstate_virtio_gpu_scanouts, g,NULL ); } static int virtio_gpu_load(QEMUFile *f, void *opaque, size_t size,hw/virtio/virtio.c +7
-6@@ -1899,7 +1899,7 @@ static const VMStateDescription vmstate_virtio = { } }; -void virtio_save (VirtIODevice *vdev,QEMUFile *f) +int virtio_save (VirtIODevice *vdev,QEMUFile *f) {BusState *qbus =qdev_get_parent_bus (DEVICE (vdev));VirtioBusClass *k =VIRTIO_BUS_GET_CLASS (qbus);@@ -1949,20 +1949,21 @@ void virtio_save(VirtIODevice *vdev, QEMUFile *f) }if (vdc->vmsd) { -vmstate_save_state (f, vdc->vmsd, vdev,NULL ); +int ret =vmstate_save_state (f, vdc->vmsd, vdev,NULL ); +if (ret) { +return ret; + } }/* Subsections */ -vmstate_save_state (f, &vmstate_virtio, vdev,NULL ); +return vmstate_save_state (f, &vmstate_virtio, vdev,NULL ); }/* A wrapper for use as a VMState .put function */ static int virtio_device_put (QEMUFile *f,void *opaque,size_t size,VMStateField *field,QJSON *vmdesc) { -virtio_save (VIRTIO_DEVICE (opaque), f); - -return 0 ; +return virtio_save (VIRTIO_DEVICE (opaque), f); }/* A wrapper for use as a VMState .get function */ include/hw/virtio/virtio.h +1
-1@@ -188,7 +188,7 @@ void virtqueue_get_avail_bytes(VirtQueue *vq, unsigned int *in_bytes,void virtio_notify_irqfd (VirtIODevice *vdev,VirtQueue *vq);void virtio_notify (VirtIODevice *vdev,VirtQueue *vq); -void virtio_save (VirtIODevice *vdev,QEMUFile *f); +int virtio_save (VirtIODevice *vdev,QEMUFile *f);extern const VMStateInfo virtio_vmstate_info;migration/vmstate-types.c +8
-3@@ -550,13 +550,14 @@ static int put_tmp(QEMUFile *f, void *pv, size_t size, VMStateField *field, {const VMStateDescription *vmsd = field->vmsd;void *tmp =g_malloc (size); +int ret;/* Writes the parent field which is at the start of the tmp */ *(void **)tmp = pv; -vmstate_save_state (f, vmsd, tmp, vmdesc); + ret =vmstate_save_state (f, vmsd, tmp, vmdesc);g_free (tmp); -return 0 ; +return ret; }const VMStateInfo vmstate_info_tmp = {@@ -657,12 +658,16 @@ static int put_qtailq(QEMUFile *f, void *pv, size_t unused_size,/* offset of the QTAILQ entry in a QTAILQ element*/ size_t entry_offset = field->start;void *elm; +int ret;trace_put_qtailq (vmsd->name, vmsd->version_id); QTAILQ_RAW_FOREACH(elm, pv, entry_offset) {qemu_put_byte (f, true); -vmstate_save_state (f, vmsd, elm, vmdesc); + ret =vmstate_save_state (f, vmsd, elm, vmdesc); +if (ret) { +return ret; + } }qemu_put_byte (f, false);tests/test-vmstate.c +3
-3@@ -70,7 +70,7 @@ static void save_vmstate(const VMStateDescription *desc, void *obj)QEMUFile *f =open_test_file (true);/* Save file with vmstate */ -vmstate_save_state (f, desc, obj,NULL ); +g_assert (!vmstate_save_state (f, desc, obj,NULL ));qemu_put_byte (f,QEMU_VM_EOF );g_assert (!qemu_file_get_error (f));qemu_fclose (f);@@ -381,7 +381,7 @@ static void test_save_noskip(void)QEMUFile *fsave =open_test_file (true);TestStruct obj = { .a =1 , .b =2 , .c =3 , .d =4 , .e =5 , .f =6 , .skip_c_e = false }; -vmstate_save_state (fsave, &vmstate_skipping, &obj,NULL ); +g_assert (!vmstate_save_state (fsave, &vmstate_skipping, &obj,NULL ));g_assert (!qemu_file_get_error (fsave));uint8_t expected[] = {@@ -402,7 +402,7 @@ static void test_save_skip(void)QEMUFile *fsave =open_test_file (true);TestStruct obj = { .a =1 , .b =2 , .c =3 , .d =4 , .e =5 , .f =6 , .skip_c_e = true }; -vmstate_save_state (fsave, &vmstate_skipping, &obj,NULL ); +g_assert (!vmstate_save_state (fsave, &vmstate_skipping, &obj,NULL ));g_assert (!qemu_file_get_error (fsave));uint8_t expected[] = {Signature · 1 line
2.13.5
Signature · 1 line
Peter Xu
Re: [Qemu-devel] [PATCH 5/5] migration: Route more error paths
Quoted 6 lines · * Peter Xu ([email protected]) wrote:
Quoted 5 lines · Dr. David Alan Gilbert (git)
From: "Dr. David Alan Gilbert" <[email protected]>
vmstate_saveis called in a few places, andvmstate_save_stateis called in lots of places.
Quoted 7 lines
Quoted 4 lines
Route error returns from the easier cases back up; there are lots of more complex cases where there own error paths need fixing.
Signed-off-by Dr. David Alan Gilbert <[email protected]>IIUC we are touching up the put()s hooks. Then do we need this as well below?
You're right - 'put' has returned int since Jianjun's patches but I seem to have missed that it never wired the check up.
Quoted 19 lines
-----
migration/vmstate.c +4
-2@@ -355,7 +355,10 @@ void vmstate_save_state(QEMUFile *f, const VMStateDescription *vmsd, }else if (field->flags &VMS_STRUCT ) {vmstate_save_state (f, field->vmsd, curr_elem, vmdesc_loop); }else { - field->info->put (f, curr_elem, size, field, vmdesc_loop); + ret = field->info->put (f, curr_elem, size, field, vmdesc_loop); +if (ret) { +return ret; + } } written_bytes =qemu_ftell_fast (f) - old_offset; -----
Quoted 1 line
Or these errors won't really stop the migration?
Added as a separate patch (with an error report in the if)
Thanks,
Dave
Quoted 146 lines
Quoted 146 lines
--- hw/display/virtio-gpu.c | 4 +--- hw/virtio/virtio.c | 13 +++++++------ include/hw/virtio/virtio.h | 2 +- migration/vmstate-types.c | 11 ++++++++--- tests/test-vmstate.c | 6 +++--- 5 files changed, 20 insertions(+), 16 deletions(-)hw/display/virtio-gpu.c +1
-3@@ -996,9 +996,7 @@ static int virtio_gpu_save(QEMUFile *f, void *opaque, size_t size, }qemu_put_be32 (f,0 );/* end of list */ -vmstate_save_state (f, &vmstate_virtio_gpu_scanouts, g,NULL ); - -return 0 ; +return vmstate_save_state (f, &vmstate_virtio_gpu_scanouts, g,NULL ); } static int virtio_gpu_load(QEMUFile *f, void *opaque, size_t size,hw/virtio/virtio.c +7
-6@@ -1899,7 +1899,7 @@ static const VMStateDescription vmstate_virtio = { } }; -void virtio_save (VirtIODevice *vdev,QEMUFile *f) +int virtio_save (VirtIODevice *vdev,QEMUFile *f) {BusState *qbus =qdev_get_parent_bus (DEVICE (vdev));VirtioBusClass *k =VIRTIO_BUS_GET_CLASS (qbus);@@ -1949,20 +1949,21 @@ void virtio_save(VirtIODevice *vdev, QEMUFile *f) }if (vdc->vmsd) { -vmstate_save_state (f, vdc->vmsd, vdev,NULL ); +int ret =vmstate_save_state (f, vdc->vmsd, vdev,NULL ); +if (ret) { +return ret; + } }/* Subsections */ -vmstate_save_state (f, &vmstate_virtio, vdev,NULL ); +return vmstate_save_state (f, &vmstate_virtio, vdev,NULL ); }/* A wrapper for use as a VMState .put function */ static int virtio_device_put (QEMUFile *f,void *opaque,size_t size,VMStateField *field,QJSON *vmdesc) { -virtio_save (VIRTIO_DEVICE (opaque), f); - -return 0 ; +return virtio_save (VIRTIO_DEVICE (opaque), f); }/* A wrapper for use as a VMState .get function */ include/hw/virtio/virtio.h +1
-1@@ -188,7 +188,7 @@ void virtqueue_get_avail_bytes(VirtQueue *vq, unsigned int *in_bytes,void virtio_notify_irqfd (VirtIODevice *vdev,VirtQueue *vq);void virtio_notify (VirtIODevice *vdev,VirtQueue *vq); -void virtio_save (VirtIODevice *vdev,QEMUFile *f); +int virtio_save (VirtIODevice *vdev,QEMUFile *f);extern const VMStateInfo virtio_vmstate_info;migration/vmstate-types.c +8
-3@@ -550,13 +550,14 @@ static int put_tmp(QEMUFile *f, void *pv, size_t size, VMStateField *field, {const VMStateDescription *vmsd = field->vmsd;void *tmp =g_malloc (size); +int ret;/* Writes the parent field which is at the start of the tmp */ *(void **)tmp = pv; -vmstate_save_state (f, vmsd, tmp, vmdesc); + ret =vmstate_save_state (f, vmsd, tmp, vmdesc);g_free (tmp); -return 0 ; +return ret; }const VMStateInfo vmstate_info_tmp = {@@ -657,12 +658,16 @@ static int put_qtailq(QEMUFile *f, void *pv, size_t unused_size,/* offset of the QTAILQ entry in a QTAILQ element*/ size_t entry_offset = field->start;void *elm; +int ret;trace_put_qtailq (vmsd->name, vmsd->version_id); QTAILQ_RAW_FOREACH(elm, pv, entry_offset) {qemu_put_byte (f, true); -vmstate_save_state (f, vmsd, elm, vmdesc); + ret =vmstate_save_state (f, vmsd, elm, vmdesc); +if (ret) { +return ret; + } }qemu_put_byte (f, false);tests/test-vmstate.c +3
-3@@ -70,7 +70,7 @@ static void save_vmstate(const VMStateDescription *desc, void *obj)QEMUFile *f =open_test_file (true);/* Save file with vmstate */ -vmstate_save_state (f, desc, obj,NULL ); +g_assert (!vmstate_save_state (f, desc, obj,NULL ));qemu_put_byte (f,QEMU_VM_EOF );g_assert (!qemu_file_get_error (f));qemu_fclose (f);@@ -381,7 +381,7 @@ static void test_save_noskip(void)QEMUFile *fsave =open_test_file (true);TestStruct obj = { .a =1 , .b =2 , .c =3 , .d =4 , .e =5 , .f =6 , .skip_c_e = false }; -vmstate_save_state (fsave, &vmstate_skipping, &obj,NULL ); +g_assert (!vmstate_save_state (fsave, &vmstate_skipping, &obj,NULL ));g_assert (!qemu_file_get_error (fsave));uint8_t expected[] = {@@ -402,7 +402,7 @@ static void test_save_skip(void)QEMUFile *fsave =open_test_file (true);TestStruct obj = { .a =1 , .b =2 , .c =3 , .d =4 , .e =5 , .f =6 , .skip_c_e = true }; -vmstate_save_state (fsave, &vmstate_skipping, &obj,NULL ); +g_assert (!vmstate_save_state (fsave, &vmstate_skipping, &obj,NULL ));g_assert (!qemu_file_get_error (fsave));
Quoted 7 lines
Quoted 4 lines
tests/test-vmstate.c +0
-0uint8_t expected[] = {Signature · 1 line
2.13.5Signature · 1 line
Peter Xu
--
Dr. David Alan Gilbert / [email protected] / Manchester, UK
Re: [Qemu-devel] [PATCH 5/5] migration: Route more error paths
In reply to Dr. David Alan Gilbert (git)Quoted 7 lines · Dr. David Alan Gilbert (git)
From: "Dr. David Alan Gilbert" <[email protected]>
vmstate_saveis called in a few places, andvmstate_save_stateis called in lots of places.Route error returns from the easier cases back up; there are lots of more complex cases where there own error paths need fixing.
Did you mean s/there/their/ ?
<snip>
tests/test-vmstate.c static void save_vmstate(const VMStateDescription *desc, void *obj) · line 73
Quoted 10 lines
tests/test-vmstate.c +1
-1@@ -70,7 +70,7 @@ static void save_vmstate(const VMStateDescription *desc, void *obj)QEMUFile *f =open_test_file (true);/* Save file with vmstate */ -vmstate_save_state (f, desc, obj,NULL ); +g_assert (!vmstate_save_state (f, desc, obj,NULL ));
Though this is test code, isn't putting anything with a side effect into an assert expression a very bad pattern in general?
Quoted 25 lines
tests/test-vmstate.c +2
-2qemu_put_byte (f,QEMU_VM_EOF );g_assert (!qemu_file_get_error (f));qemu_fclose (f);@@ -381,7 +381,7 @@ static void test_save_noskip(void)QEMUFile *fsave =open_test_file (true);TestStruct obj = { .a =1 , .b =2 , .c =3 , .d =4 , .e =5 , .f =6 , .skip_c_e = false }; -vmstate_save_state (fsave, &vmstate_skipping, &obj,NULL ); +g_assert (!vmstate_save_state (fsave, &vmstate_skipping, &obj,NULL ));g_assert (!qemu_file_get_error (fsave));uint8_t expected[] = {@@ -402,7 +402,7 @@ static void test_save_skip(void)QEMUFile *fsave =open_test_file (true);TestStruct obj = { .a =1 , .b =2 , .c =3 , .d =4 , .e =5 , .f =6 , .skip_c_e = true }; -vmstate_save_state (fsave, &vmstate_skipping, &obj,NULL ); +g_assert (!vmstate_save_state (fsave, &vmstate_skipping, &obj,NULL ));g_assert (!qemu_file_get_error (fsave));uint8_t expected[] = {Signature · 1 line
2.13.5
Fam
Re: [Qemu-devel] [PATCH 5/5] migration: Route more error paths
Quoted 14 lines · Fam Zheng
Quoted 13 lines · Dr. David Alan Gilbert (git)
From: "Dr. David Alan Gilbert" <[email protected]>
vmstate_saveis called in a few places, andvmstate_save_stateis called in lots of places.Route error returns from the easier cases back up; there are lots of more complex cases where there own error paths need fixing.
+++
b/tests/test-vmstate.ctests/test-vmstate.c +0
-0@@ -70,7 +70,7 @@ static void save_vmstate(const VMStateDescription *desc, void *obj)QEMUFile *f =open_test_file (true);
Quoted 6 lines
tests/test-vmstate.c static void save_vmstate(const VMStateDescription *desc, void *obj) · line 73
Quoted 3 lines
tests/test-vmstate.c +1
-1/* Save file with vmstate */ -vmstate_save_state (f, desc, obj,NULL ); +g_assert (!vmstate_save_state (f, desc, obj,NULL ));Though this is test code, isn't putting anything with a side effect into an assert expression a very bad pattern in general?
Indeed - although we don't disable asserts (as of commit 262a69f4), this should still be separated into running vmstate_save_state() unconditionally, then asserting that the result stored into a temporary variable matches expectations.
Signature · 3 lines
Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [Qemu-devel] [PATCH 5/5] migration: Route more error paths
In reply to Fam ZhengQuoted 10 lines · * Fam Zheng ([email protected]) wrote:
Quoted 7 lines · Dr. David Alan Gilbert (git)
From: "Dr. David Alan Gilbert" <[email protected]>
vmstate_saveis called in a few places, andvmstate_save_stateis called in lots of places.Route error returns from the easier cases back up; there are lots of more complex cases where there own error paths need fixing.
Did you mean s/there/their/ ?
I do; thanks.
Quoted 9 lines
<snip>
Quoted 7 lines
tests/test-vmstate.c +0
-0@@ -70,7 +70,7 @@ static void save_vmstate(const VMStateDescription *desc, void *obj)QEMUFile *f =open_test_file (true);
Quoted 6 lines
tests/test-vmstate.c static void save_vmstate(const VMStateDescription *desc, void *obj) · line 73
Quoted 3 lines
tests/test-vmstate.c +1
-1/* Save file with vmstate */ -vmstate_save_state (f, desc, obj,NULL ); +g_assert (!vmstate_save_state (f, desc, obj,NULL ));Though this is test code, isn't putting anything with a side effect into an assert expression a very bad pattern in general?
Hmm; ok I've changed this but I'm not really convinced; the whole point of an asser in a test is to actually run it, and I think the g_assert prints the text that failed, so it gives you a much better error inline.
Dave
Quoted 20 lines
Quoted 20 lines
tests/test-vmstate.c +2
-2qemu_put_byte (f,QEMU_VM_EOF );g_assert (!qemu_file_get_error (f));qemu_fclose (f);@@ -381,7 +381,7 @@ static void test_save_noskip(void)QEMUFile *fsave =open_test_file (true);TestStruct obj = { .a =1 , .b =2 , .c =3 , .d =4 , .e =5 , .f =6 , .skip_c_e = false }; -vmstate_save_state (fsave, &vmstate_skipping, &obj,NULL ); +g_assert (!vmstate_save_state (fsave, &vmstate_skipping, &obj,NULL ));g_assert (!qemu_file_get_error (fsave));uint8_t expected[] = {@@ -402,7 +402,7 @@ static void test_save_skip(void)QEMUFile *fsave =open_test_file (true);TestStruct obj = { .a =1 , .b =2 , .c =3 , .d =4 , .e =5 , .f =6 , .skip_c_e = true }; -vmstate_save_state (fsave, &vmstate_skipping, &obj,NULL ); +g_assert (!vmstate_save_state (fsave, &vmstate_skipping, &obj,NULL ));g_assert (!qemu_file_get_error (fsave));
Quoted 7 lines
Quoted 5 lines
tests/test-vmstate.c +0
-0uint8_t expected[] = {Signature · 1 line
2.13.5Fam
--
Dr. David Alan Gilbert / [email protected] / Manchester, UK
Re: [Qemu-devel] [PATCH 5/5] migration: Route more error paths
Quoted 10 lines · Dr. David Alan Gilbert
Quoted 6 lines
tests/test-vmstate.c static void save_vmstate(const VMStateDescription *desc, void *obj) · line 73
Quoted 3 lines
/* Save file with vmstate */tests/test-vmstate.c +1
-1-vmstate_save_state (f, desc, obj,NULL ); +g_assert (!vmstate_save_state (f, desc, obj,NULL ));Though this is test code, isn't putting anything with a side effect into an assert expression a very bad pattern in general?
Hmm; ok I've changed this but I'm not really convinced; the whole point of an asser in a test is to actually run it, and I think the
g_assertprints the text that failed, so it gives you a much better error inline.
glib doesn't make life any easier by having two different flavors of assertions.
g_assert() can be disabled at compile-time (side effects are lost). It is roughly akin to assert(); and the output when an assertion fails isn't that much more useful than what you get with plain assert().
g_assert_cmpint() cannot be disabled at compile-time, but can have a runtime switch on whether it abort()s or continues on. Here, the output when something fails is MUCH more pleasant to read.
But the fact that there are two separate families of g_assert* behaviors just makes it easier to live by the mantra that side-effects don't belong inside assertions.
Signature · 3 lines
Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [Qemu-devel] [PATCH 5/5] migration: Route more error paths
In reply to Dr. David Alan Gilbert (git)Quoted 23 lines · On Tue, 19 Sep 2017 19:00:38 +0100 "Dr. David Alan Gilbert (git)"…
From: "Dr. David Alan Gilbert" <[email protected]>
vmstate_saveis called in a few places, andvmstate_save_stateis called in lots of places.Route error returns from the easier cases back up; there are lots of more complex cases where there own error paths need fixing.
Signed-off-by Dr. David Alan Gilbert <[email protected]>--- hw/display/virtio-gpu.c | 4 +--- hw/virtio/virtio.c | 13 +++++++------ include/hw/virtio/virtio.h | 2 +- migration/vmstate-types.c | 11 ++++++++--- tests/test-vmstate.c | 6 +++--- 5 files changed, 20 insertions(+), 16 deletions(-)hw/virtio/virtio.c +0
-0
hw/virtio/virtio.c static const VMStateDescription vmstate_virtio = { · line 1905
Quoted 8 lines
hw/virtio/virtio.c +1
-1@@ -1899,7 +1899,7 @@ static const VMStateDescription vmstate_virtio = { } }; -void virtio_save (VirtIODevice *vdev,QEMUFile *f) +int virtio_save (VirtIODevice *vdev,QEMUFile *f) {BusState *qbus =qdev_get_parent_bus (DEVICE (vdev));VirtioBusClass *k =VIRTIO_BUS_GET_CLASS (qbus);
Would it make sense to touch up the save_config callback as well? Else virtio_save() looks a bit lopsided.
[For virtio-ccw, the callback can simply pass on any return code from vmstate_save_state(). For virtio-pci, we can probably make pci_device_save() restore the interrupt state in any case.]
Re: [Qemu-devel] [PATCH 5/5] migration: Route more error paths
Quoted 38 lines · * Cornelia Huck ([email protected]) wrote:
Quoted 23 lines · On Tue, 19 Sep 2017 19:00:38 +0100 "Dr. David Alan Gilbert (git)"…
From: "Dr. David Alan Gilbert" <[email protected]>
vmstate_saveis called in a few places, andvmstate_save_stateis called in lots of places.Route error returns from the easier cases back up; there are lots of more complex cases where there own error paths need fixing.
Signed-off-by Dr. David Alan Gilbert <[email protected]>--- hw/display/virtio-gpu.c | 4 +--- hw/virtio/virtio.c | 13 +++++++------ include/hw/virtio/virtio.h | 2 +- migration/vmstate-types.c | 11 ++++++++--- tests/test-vmstate.c | 6 +++--- 5 files changed, 20 insertions(+), 16 deletions(-)hw/virtio/virtio.c +0
-0hw/virtio/virtio.c static const VMStateDescription vmstate_virtio = { · line 1905
Quoted 8 lines
hw/virtio/virtio.c +1
-1@@ -1899,7 +1899,7 @@ static const VMStateDescription vmstate_virtio = { } }; -void virtio_save (VirtIODevice *vdev,QEMUFile *f) +int virtio_save (VirtIODevice *vdev,QEMUFile *f) {BusState *qbus =qdev_get_parent_bus (DEVICE (vdev));VirtioBusClass *k =VIRTIO_BUS_GET_CLASS (qbus);Would it make sense to touch up the
save_configcallback as well? Elsevirtio_save()looks a bit lopsided.
Quoted 3 lines
[For virtio-ccw, the callback can simply pass on any return code from
vmstate_save_state(). For virtio-pci, we can probably makepci_device_save()restore the interrupt state in any case.]
What I'd really love to do with save_config is turn it into a VMStateDescription* - I think I've figured out how to do that for ccw and mmio - but the pci version is a much harder mess of casts and stuff.
Dave
--
Dr. David Alan Gilbert / [email protected] / Manchester, UK
Re: [Qemu-devel] [PATCH 5/5] migration: Route more error paths
Quoted 39 lines · On Wed, 20 Sep 2017 16:57:39 +0100 "Dr. David Alan Gilbert"…
Quoted 38 lines · * Cornelia Huck ([email protected]) wrote:
Quoted 23 lines · On Tue, 19 Sep 2017 19:00:38 +0100 "Dr. David Alan Gilbert (git)"…
From: "Dr. David Alan Gilbert" <[email protected]>
vmstate_saveis called in a few places, andvmstate_save_stateis called in lots of places.Route error returns from the easier cases back up; there are lots of more complex cases where there own error paths need fixing.
Signed-off-by Dr. David Alan Gilbert <[email protected]>--- hw/display/virtio-gpu.c | 4 +--- hw/virtio/virtio.c | 13 +++++++------ include/hw/virtio/virtio.h | 2 +- migration/vmstate-types.c | 11 ++++++++--- tests/test-vmstate.c | 6 +++--- 5 files changed, 20 insertions(+), 16 deletions(-)hw/virtio/virtio.c +0
-0hw/virtio/virtio.c static const VMStateDescription vmstate_virtio = { · line 1905
Quoted 8 lines
hw/virtio/virtio.c +1
-1@@ -1899,7 +1899,7 @@ static const VMStateDescription vmstate_virtio = { } }; -void virtio_save (VirtIODevice *vdev,QEMUFile *f) +int virtio_save (VirtIODevice *vdev,QEMUFile *f) {BusState *qbus =qdev_get_parent_bus (DEVICE (vdev));VirtioBusClass *k =VIRTIO_BUS_GET_CLASS (qbus);Would it make sense to touch up the
save_configcallback as well? Elsevirtio_save()looks a bit lopsided.
Quoted 8 lines
Quoted 3 lines
[For virtio-ccw, the callback can simply pass on any return code from
vmstate_save_state(). For virtio-pci, we can probably makepci_device_save()restore the interrupt state in any case.]What I'd really love to do with
save_configis turn it into a VMStateDescription* - I think I've figured out how to do that for ccw and mmio - but the pci version is a much harder mess of casts and stuff.
Sounds even better, but even more complicated as well...