make windows notice media change
PATCH 1/1 make windows notice media change
Windows seems to be very stupid about cdrom media change. It polls cdrom status and if status goes ready->media not present->ready it assumes that media was changed. If "media not present" step doesn't happen even if "medium may have changed" was seen it assumes media haven't changed. Fake "media not present" step.
hw/ide.c +3 -2
@@ -1644,9 +1644,10 @@ static void ide_atapi_cmd(IDEState *s) }switch (s->io_buffer[0 ]) {case GPCMD_TEST_UNIT_READY : -if (bdrv_is_inserted (s->bs)) { +if (bdrv_is_inserted (s->bs) && !s->media_changed) {ide_atapi_cmd_ok (s); }else { + s->media_changed =0 ;ide_atapi_cmd_error (s,SENSE_NOT_READY ,ASC_MEDIUM_NOT_PRESENT ); }
@@ -2106,7 +2107,7 @@ static void cdrom_change_cb(void *opaque) s->sense_key =SENSE_UNIT_ATTENTION ; s->asc =ASC_MEDIUM_MAY_HAVE_CHANGED ; - + s->media_changed =1 ;ide_set_irq (s); }
--
Gleb.
PATCH v2 1/1 make windows notice media change
Windows seems to be very stupid about cdrom media change. It polls cdrom status and if status goes ready->media not present->ready it assumes that media was changed. If "media not present" step doesn't happen even if "medium may have changed" was seen it assumes media haven't changed. Fake "media not present" step.
---v1->v2:
do not reuse media_changed. Save/restore on migration.
hw/ide.c +8 -4
@@ -418,6 +418,7 @@ typedef struct IDEState {/* ATAPI specific */ uint8_t sense_key;uint8_t asc; +uint8_t cdrom_changed;int packet_transfer_size;int elementary_transfer_size;int io_buffer_index;
@@ -1644,9 +1645,10 @@ static void ide_atapi_cmd(IDEState *s) }switch (s->io_buffer[0 ]) {case GPCMD_TEST_UNIT_READY : -if (bdrv_is_inserted (s->bs)) { +if (bdrv_is_inserted (s->bs) && !s->cdrom_changed) {ide_atapi_cmd_ok (s); }else { + s->cdrom_changed =0 ;ide_atapi_cmd_error (s,SENSE_NOT_READY ,ASC_MEDIUM_NOT_PRESENT ); }
@@ -2106,7 +2108,7 @@ static void cdrom_change_cb(void *opaque) s->sense_key =SENSE_UNIT_ATTENTION ; s->asc =ASC_MEDIUM_MAY_HAVE_CHANGED ; - + s->cdrom_changed =1 ;ide_set_irq (s); }
@@ -2870,6 +2872,7 @@ static void ide_save(QEMUFile* f, IDEState *s)qemu_put_8s (f, &s->sense_key);qemu_put_8s (f, &s->asc); +qemu_put_8s (f, &s->cdrom_changed);/* XXX: if a transfer is pending, we do not save it yet */ }
@@ -2898,6 +2901,7 @@ static void ide_load(QEMUFile* f, IDEState *s)qemu_get_8s (f, &s->sense_key);qemu_get_8s (f, &s->asc); +qemu_get_8s (f, &s->cdrom_changed);/* XXX: if a transfer is pending, we do not save it yet */ }
@@ -3219,7 +3223,7 @@ static int pci_ide_load(QEMUFile* f, void *opaque, int version_id)PCIIDEState *d = opaque;int ret, i; -if (version_id !=2 ) +if (version_id !=3 )return -EINVAL ; ret =pci_device_load (&d->dev, f);if (ret <0 )
@@ -3339,7 +3343,7 @@ void pci_cmd646_ide_init(PCIBus *bus, BlockDriverState **hd_table,ide_init2 (&d->ide_if[0 ], hd_table[0 ], hd_table[1 ], irq[0 ]);ide_init2 (&d->ide_if[2 ], hd_table[2 ], hd_table[3 ], irq[1 ]); -register_savevm ("ide" ,0 ,2 , pci_ide_save, pci_ide_load, d); +register_savevm ("ide" ,0 ,3 , pci_ide_save, pci_ide_load, d);qemu_register_reset (cmd646_reset, d);cmd646_reset (d); }
--
Gleb.
hw/ide.c static void ide_load(QEMUFile* f, IDEState *s) · line 2907
Quoted 9 lines · Gleb Natapov
hw/ide.c +1
-0@@ -2898,6 +2901,7 @@ static void ide_load(QEMUFile* f, IDEState *s)qemu_get_8s (f,&s->sense_key);qemu_get_8s (f,&s->asc); +qemu_get_8s (f,&s->cdrom_changed);/* XXX: if a transfer is pending, we do not save it yet */ }
Needs to be conditional on version, no?
Signature · 1 line
error compiling committee.c: too many arguments to function
Quoted 13 lines · Avi Kivity
hw/ide.c static void ide_load(QEMUFile* f, IDEState *s) · line 2907
Quoted 9 lines · Gleb Natapov
hw/ide.c +1
-0@@ -2898,6 +2901,7 @@ static void ide_load(QEMUFile* f, IDEState *s)qemu_get_8s (f,&s->sense_key);qemu_get_8s (f,&s->asc); +qemu_get_8s (f,&s->cdrom_changed);/* XXX: if a transfer is pending, we do not save it yet */ }Needs to be conditional on version, no?
I changed version check in vmload.
--
Gleb.
Quoted 17 lines · Gleb Natapov
Quoted 4 lines · Avi Kivity
Quoted 2 lines · Gleb Natapov
(quoted diff) +0
-0@@ -2898,6 +2901,7 @@ static void ide_load(QEMUFile* f, IDEState *s)Quoted 11 lines
hw/ide.c static void ide_save(QEMUFile* f, IDEState *s) · line 2878
Quoted 8 lines
hw/ide.c +1
-0qemu_get_8s (f,&s->sense_key);qemu_get_8s (f,&s->asc); +qemu_get_8s (f,&s->cdrom_changed);/* XXX: if a transfer is pending, we do not save it yet */ }Needs to be conditional on version, no?
Quoted 3 lines
I changed version check in vmload.
Don't we usually allow loading from older versions?
Signature · 1 line
error compiling committee.c: too many arguments to function
On Wed, Jul 29, 2009 at 3:50 PM, Gleb Natapov<[email protected]> wrote:
[snip]
Quoted 9 lines
hw/ide.c +1
-0@@ -2898,6 +2901,7 @@ static void ide_load(QEMUFile* f, IDEState *s) qemu_get_8s(f, &s->sense_key); qemu_get_8s(f, &s->asc); + qemu_get_8s(f, &s->cdrom_changed); /* XXX: if a transfer is pending, we do not save it yet */ }
hw/ide.c static int pci_ide_load(QEMUFile* f, void *opaque, int version_id) · line 3229
Quoted 8 lines
hw/ide.c +1
-1@@ -3219,7 +3223,7 @@ static int pci_ide_load(QEMUFile* f, void *opaque, int version_id) PCIIDEState *d = opaque; int ret, i; - if (version_id != 2) + if (version_id != 3) return -EINVAL; ret = pci_device_load(&d->dev, f); if (ret < 0)
Why shouldn't we support loading version 2 snapshots? Afterall that's why we had the versioning in the first place.
F.
Quoted 14 lines · Filip Navara
On Wed, Jul 29, 2009 at 3:50 PM, Gleb Natapov<[email protected]> wrote:
[snip]Quoted 12 lines
hw/ide.c +1
-0@@ -2898,6 +2901,7 @@ static void ide_load(QEMUFile* f, IDEState *s) qemu_get_8s(f, &s->sense_key); qemu_get_8s(f, &s->asc); + qemu_get_8s(f, &s->cdrom_changed); /* XXX: if a transfer is pending, we do not save it yet */ }@@ -3219,7 +3223,7 @@ static int pci_ide_load(QEMUFile* f, void *opaque, int version_id) PCIIDEState *d = opaque; int ret, i;
Quoted 9 lines
hw/ide.c static int pci_ide_load(QEMUFile* f, void *opaque, int version_id) · line 3229
Quoted 5 lines
hw/ide.c +1
-1- if (version_id != 2) + if (version_id != 3) return -EINVAL; ret = pci_device_load(&d->dev, f); if (ret < 0)Why shouldn't we support loading version 2 snapshots? Afterall that's why we had the versioning in the first place.
Does anybody check cross version migration? If no my guess is it is broken. And I can't even imaging test matrix. Versioning is a good way to prevent users from doing stupid things.
--
Gleb.
Quoted 24 lines · Gleb Natapov
Quoted 14 lines · Filip Navara
On Wed, Jul 29, 2009 at 3:50 PM, Gleb Natapov<[email protected]> wrote:
[snip]Quoted 12 lines
hw/ide.c +1
-0@@ -2898,6 +2901,7 @@ static void ide_load(QEMUFile* f, IDEState *s) qemu_get_8s(f, &s->sense_key); qemu_get_8s(f, &s->asc); + qemu_get_8s(f, &s->cdrom_changed); /* XXX: if a transfer is pending, we do not save it yet */ }@@ -3219,7 +3223,7 @@ static int pci_ide_load(QEMUFile* f, void *opaque, int version_id) PCIIDEState *d = opaque; int ret, i;Quoted 9 lines
hw/ide.c static int pci_ide_load(QEMUFile* f, void *opaque, int version_id) · line 3229
Quoted 5 lines
hw/ide.c +1
-1- if (version_id != 2) + if (version_id != 3) return -EINVAL; ret = pci_device_load(&d->dev, f); if (ret < 0)Why shouldn't we support loading version 2 snapshots? Afterall that's why we had the versioning in the first place.
Quoted 4 lines
Does anybody check cross version migration? If no my guess is it is broken. And I can't even imaging test matrix. Versioning is a good way to prevent users from doing stupid things.
We do support cross version migration, only from and older to a newer release.
Quoted 2 lines
Why shouldn't we support loading version 2 snapshots? Afterall that's why we had the versioning in the first place.
No it's not. Versioning was introduced to *prevent* loading old snapshots and crashing or ending up with inconsistent guest state. I'm still unconvinced that anything other than very short term backward compatibility is worthwhile or even viable.
Paul
Quoted 7 lines · Paul Brook
Quoted 2 lines
Why shouldn't we support loading version 2 snapshots? Afterall that's why we had the versioning in the first place.
No it's not. Versioning was introduced to *prevent* loading old snapshots and crashing or ending up with inconsistent guest state. I'm still unconvinced that anything other than very short term backward compatibility is worthwhile or even viable.
I see it as a way to migrate a running guest to newer QEMU version, possibly even with live migration. In fact I used it quite often back in the day when snapshots were not part of qcow2 yet and when kqemu was still in its heydays.
Best regards,
Filip Navara
Quoted 4 lines · Filip Navara
Quoted 3 lines · Paul Brook
Quoted 2 lines
Why shouldn't we support loading version 2 snapshots? Afterall that's why we had the versioning in the first place.
Quoted 9 lines
Quoted 4 lines
No it's not. Versioning was introduced to *prevent* loading old snapshots and crashing or ending up with inconsistent guest state. I'm still unconvinced that anything other than very short term backward compatibility is worthwhile or even viable.
I see it as a way to migrate a running guest to newer QEMU version, possibly even with live migration. In fact I used it quite often back in the day when snapshots were not part of qcow2 yet and when kqemu was still in its heydays.
BTW, why would there be the version parameter in the first place if it wasn't supposed to load older versions?!
Best regards,
Filip Navara
Quoted 12 lines · Filip Navara
Quoted 10 lines · Filip Navara
Quoted 8 lines · Paul Brook
Quoted 3 lines
Why shouldn't we support loading version 2 snapshots? Afterall that's why we had the versioning in the first place.
No it's not. Versioning was introduced to *prevent* loading old snapshots and crashing or ending up with inconsistent guest state. I'm still unconvinced that anything other than very short term backward compatibility is worthwhile or even viable.
Quoted 9 lines
Quoted 5 lines
I see it as a way to migrate a running guest to newer QEMU version, possibly even with live migration. In fact I used it quite often back in the day when snapshots were not part of qcow2 yet and when kqemu was still in its heydays.
BTW, why would there be the version parameter in the first place if it wasn't supposed to load older versions?!
You can sometimes load older versions. When a new version fixes a bug in an older version, you cannot migrate. OTOH, if the new version adds a feature that was not exposed to the guest in the previous version, as long as you continue to not expose that feature to the guest, you can safely migrate.
Regards,
Anthony Liguori
Quoted 8 lines
Quoted 8 lines
Quoted 7 lines
Quoted 2 lines
Why shouldn't we support loading version 2 snapshots? Afterall that's why we had the versioning in the first place.
No it's not. Versioning was introduced to *prevent* loading old snapshots and crashing or ending up with inconsistent guest state. I'm still unconvinced that anything other than very short term backward compatibility is worthwhile or even viable.
Quoted 7 lines
Quoted 4 lines
I see it as a way to migrate a running guest to newer QEMU version, possibly even with live migration. In fact I used it quite often back in the day when snapshots were not part of qcow2 yet and when kqemu was still in its heydays.
BTW, why would there be the version parameter in the first place if it wasn't supposed to load older versions?!
Like I already said: it's there to prevent an old version being loaded accidentally. Without this an incompatible change will result in anything from a crash to corrupt/inconsistent guest state. Versioning allows us to reject the snapshot and fail safely.
Paul
Quoted 13 lines · Paul Brook
Quoted 13 lines
Quoted 12 lines
Quoted 7 lines
Quoted 2 lines
Why shouldn't we support loading version 2 snapshots? Afterall that's why we had the versioning in the first place.
No it's not. Versioning was introduced to *prevent* loading old snapshots and crashing or ending up with inconsistent guest state. I'm still unconvinced that anything other than very short term backward compatibility is worthwhile or even viable.
I see it as a way to migrate a running guest to newer QEMU version, possibly even with live migration. In fact I used it quite often back in the day when snapshots were not part of qcow2 yet and when kqemu was still in its heydays.
Quoted 7 lines
Quoted 2 lines
BTW, why would there be the version parameter in the first place if it wasn't supposed to load older versions?!
Like I already said: it's there to prevent an old version being loaded accidentally. Without this an incompatible change will result in anything from a crash to corrupt/inconsistent guest state. Versioning allows us to reject the snapshot and fail safely.
If that was the case then the if (version != x) return -EINVAL check could have been in the generic code and there would be no need for the version parameter in the load function.
- F.
Quoted 11 lines
Quoted 7 lines
Quoted 2 lines
BTW, why would there be the version parameter in the first place if it wasn't supposed to load older versions?!
Like I already said: it's there to prevent an old version being loaded accidentally. Without this an incompatible change will result in anything from a crash to corrupt/inconsistent guest state. Versioning allows us to reject the snapshot and fail safely.
If that was the case then the if (version != x) return
-EINVALcheck could have been in the generic code and there would be no need for the version parameter in the load function.
Preventing loading bad snapshots is the primary goal.
Allowing loading old snapshots is a secondary feature. Personally I think it's not worth the effort, and in practice is unlikely to be feasible for whole machines over any significant length of time. However I don't feel strongly enough to actually rip the code out.
Paul
Quoted 12 lines · Paul Brook
Quoted 11 lines
Quoted 7 lines
Quoted 2 lines
BTW, why would there be the version parameter in the first place if it wasn't supposed to load older versions?!
Like I already said: it's there to prevent an old version being loaded accidentally. Without this an incompatible change will result in anything from a crash to corrupt/inconsistent guest state. Versioning allows us to reject the snapshot and fail safely.
If that was the case then the if (version != x) return
-EINVALcheck could have been in the generic code and there would be no need for the version parameter in the load function.
Quoted 7 lines
Preventing loading bad snapshots is the primary goal.
Allowing loading old snapshots is a secondary feature. Personally I think it's not worth the effort, and in practice is unlikely to be feasible for whole machines over any significant length of time. However I don't feel strongly enough to actually rip the code out.
Agree. All this "migrate from older version" just produce code that is never tested in practice.
--
Gleb.
Quoted 12 lines · Paul Brook
Quoted 11 lines
Quoted 7 lines
Quoted 2 lines
BTW, why would there be the version parameter in the first place if it wasn't supposed to load older versions?!
Like I already said: it's there to prevent an old version being loaded accidentally. Without this an incompatible change will result in anything from a crash to corrupt/inconsistent guest state. Versioning allows us to reject the snapshot and fail safely.
If that was the case then the if (version != x) return
-EINVALcheck could have been in the generic code and there would be no need for the version parameter in the load function.
Quoted 7 lines
Preventing loading bad snapshots is the primary goal.
Allowing loading old snapshots is a secondary feature. Personally I think it's not worth the effort, and in practice is unlikely to be feasible for whole machines over any significant length of time. However I don't feel strongly enough to actually rip the code out.
I completely disagree with you: loading old snapshots is a really important feature, at least for xen (and kvm) users.
Think about a sysadmin that kept his server running for the last X years in a VM using xen (or kvm) and now wants to upgrade his host. Is he supposed to reboot the VM as well?
Are we seriously suggesting that everytime the hypervisor is upgraded the sysadmin must reboot all his VMs?
Regarding the difficulty of the task: we (xen community and xenserver) accomplished this for the last 4 years with no particular problems and a relatively small developing team.
I have a patch in my patchqueue that allows a modern qemu to load snapshot from version 1 and it doesn't require the use of black magic.
Quoted 5 lines
Think about a sysadmin that kept his server running for the last X years in a VM using xen (or kvm) and now wants to upgrade his host. Is he supposed to reboot the VM as well?
Are we seriously suggesting that everytime the hypervisor is upgraded the sysadmin must reboot all his VMs?
I'd expect that stable branches would make effort to not break the snapshot/migration format.
I think it's entirely reasonable to require a VM reboot when you do a major hypervisor upgrade. I'd expect this to be the least of your problems.
Paul
Quoted 15 lines · Filip Navara
On Wed, Jul 29, 2009 at 3:50 PM, Gleb Natapov<[email protected]> wrote:
[snip]Quoted 12 lines
hw/ide.c +1
-0@@ -2898,6 +2901,7 @@ static void ide_load(QEMUFile* f, IDEState *s)qemu_get_8s (f, &s->sense_key);qemu_get_8s (f, &s->asc); +qemu_get_8s (f, &s->cdrom_changed);/* XXX: if a transfer is pending, we do not save it yet */ }@@ -3219,7 +3223,7 @@ static int pci_ide_load(QEMUFile* f, void *opaque, int version_id)PCIIDEState *d = opaque;int ret, i;
Quoted 10 lines
hw/ide.c static int pci_ide_load(QEMUFile* f, void *opaque, int version_id) · line 3229
Quoted 6 lines
hw/ide.c +1
-1-if (version_id !=2 ) +if (version_id !=3 )return -EINVAL ; ret =pci_device_load (&d->dev, f);if (ret <0 )Why shouldn't we support loading version 2 snapshots? Afterall that's why we had the versioning in the first place.
What value do you give cdrom_changed?
If you give it zero, and you happened to be in a state where the cdrom was in the process of being changed when you migrated, this bug would surface again.
In this case, it's better to refuse the old (broken) version.
Regards,
Anthony Liguori
Quoted 27 lines · Anthony Liguori
Quoted 15 lines · Filip Navara
On Wed, Jul 29, 2009 at 3:50 PM, Gleb Natapov<[email protected]> wrote:
[snip]Quoted 12 lines
hw/ide.c +1
-0@@ -2898,6 +2901,7 @@ static void ide_load(QEMUFile* f, IDEState *s)qemu_get_8s (f, &s->sense_key);qemu_get_8s (f, &s->asc); +qemu_get_8s (f, &s->cdrom_changed);/* XXX: if a transfer is pending, we do not save it yet */ }@@ -3219,7 +3223,7 @@ static int pci_ide_load(QEMUFile* f, void *opaque, int version_id)PCIIDEState *d = opaque;int ret, i;Quoted 10 lines
hw/ide.c static int pci_ide_load(QEMUFile* f, void *opaque, int version_id) · line 3229
Quoted 6 lines
hw/ide.c +1
-1-if (version_id !=2 ) +if (version_id !=3 )return -EINVAL ; ret =pci_device_load (&d->dev, f);if (ret <0 )Why shouldn't we support loading version 2 snapshots? Afterall that's why we had the versioning in the first place.
Quoted 7 lines
What value do you give
cdrom_changed?If you give it zero, and you happened to be in a state where the cdrom was in the process of being changed when you migrated, this bug would surface again.
In this case, it's better to refuse the old (broken) version.
You didn't lost anything from the old version, it already have the bug. Value that you give it: zero. If it was in the middle of the operation, it will give the same bug that when it was in the older host, and when it tells "retry", it will see the new behaviour from this on.
Later, Juan.
Quoted 3 lines
Regards,
Anthony Liguori
Quoted 3 lines · Juan Quintela
You didn't lost anything from the old version, it already have the bug. Value that you give it: zero. If it was in the middle of the operation, it will give the same bug that when it was in the older host,
Right, which is the root of the problem. You migrate to a new version of QEMU to fix bugs. If we don't fix bugs because we migrated from an old version, we end up in a strange situation resulting in strange bug reports.
Regards,
Anthony Liguori
Quoted 9 lines · Anthony Liguori
Quoted 3 lines · Juan Quintela
You didn't lost anything from the old version, it already have the bug. Value that you give it: zero. If it was in the middle of the operation, it will give the same bug that when it was in the older host,
Right, which is the root of the problem. You migrate to a new version of QEMU to fix bugs. If we don't fix bugs because we migrated from an old version, we end up in a strange situation resulting in strange bug reports.
In this very case, bug is fixed from now on. It can only happens once, at maximum, I consider it a net gain. Bug we are fixing here is that windows don't detect correctly that we have changed the media, in the old version, it can happens depending on ...., if you migrate to the new version, you can have that problem a maximum of _one_ time, after that, the value of the variable will always be right.
Later, Juan.