[Qemu-devel] [PATCH v3 1/1] atapi: make change media detection for guests easier

[Qemu-devel] [PATCH v3 1/1] atapi: make change media detection for guests easier

[Pavel Hrdina]

If you have a guest with a media in the optical drive and you change the media
the windows guest cannot properly recognize this media change.

Windows needs to detect the sense "NOT_READY with ASC_MEDIUM_NOT_PRESENT"
before we send the sense "UNIT_ATTENTION with ASC_MEDIUM_MAY_HAVE_CHANGED".

v3: remove timeout as it isn't needed anymore

v2: disable debug messages

Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
---
 hw/ide/atapi.c    | 16 +++++++++++-----
 hw/ide/core.c     | 12 ++++++++++++
 hw/ide/internal.h |  1 +
 3 files changed, 24 insertions(+), 5 deletions(-)

diff --git a/hw/ide/atapi.c b/hw/ide/atapi.c
index 685cbaa..1534afe 100644
--- a/hw/ide/atapi.c
+++ b/hw/ide/atapi.c
@@ -1124,12 +1124,18 @@ void ide_atapi_cmd(IDEState *s)
      * GET_EVENT_STATUS_NOTIFICATION to detect such tray open/close
      * states rely on this behavior.
      */
-    if (!s->tray_open && bdrv_is_inserted(s->bs) && s->cdrom_changed) {
-        ide_atapi_cmd_error(s, NOT_READY, ASC_MEDIUM_NOT_PRESENT);
+    if (!(atapi_cmd_table[s->io_buffer[0]].flags & ALLOW_UA) &&
+        !s->tray_open && bdrv_is_inserted(s->bs) && s->cdrom_changed) {
+
+        if (!s->fake_cdrom_eject) {
+            ide_atapi_cmd_error(s, NOT_READY, ASC_MEDIUM_NOT_PRESENT);
+            s->fake_cdrom_eject = 1;
+        } else {
+            ide_atapi_cmd_error(s, UNIT_ATTENTION, ASC_MEDIUM_MAY_HAVE_CHANGED);
+            s->fake_cdrom_eject = 0;
+            s->cdrom_changed = 0;
+        }
 
-        s->cdrom_changed = 0;
-        s->sense_key = UNIT_ATTENTION;
-        s->asc = ASC_MEDIUM_MAY_HAVE_CHANGED;
         return;
     }
 
diff --git a/hw/ide/core.c b/hw/ide/core.c
index 7d6b0fa..013671a 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -1851,6 +1851,7 @@ static void ide_reset(IDEState *s)
     s->sense_key = 0;
     s->asc = 0;
     s->cdrom_changed = 0;
+    s->fake_cdrom_eject = 0;
     s->packet_transfer_size = 0;
     s->elementary_transfer_size = 0;
     s->io_buffer_index = 0;
@@ -2143,6 +2144,16 @@ static int transfer_end_table_idx(EndTransferFunc *fn)
     return -1;
 }
 
+static void ide_drive_pre_save(void *opaque)
+{
+    IDEState *s = opaque;
+
+    if (s->cdrom_changed) {
+        s->sense_key = UNIT_ATTENTION;
+        s->asc = ASC_MEDIUM_MAY_HAVE_CHANGED;
+    }
+}
+
 static int ide_drive_post_load(void *opaque, int version_id)
 {
     IDEState *s = opaque;
@@ -2270,6 +2281,7 @@ const VMStateDescription vmstate_ide_drive = {
     .version_id = 3,
     .minimum_version_id = 0,
     .minimum_version_id_old = 0,
+    .pre_save = ide_drive_pre_save,
     .post_load = ide_drive_post_load,
     .fields      = (VMStateField []) {
         VMSTATE_INT32(mult_sectors, IDEState),
diff --git a/hw/ide/internal.h b/hw/ide/internal.h
index bf7d313..5fb2266 100644
--- a/hw/ide/internal.h
+++ b/hw/ide/internal.h
@@ -382,6 +382,7 @@ struct IDEState {
     bool tray_open;
     bool tray_locked;
     uint8_t cdrom_changed;
+    uint8_t fake_cdrom_eject;
     int packet_transfer_size;
     int elementary_transfer_size;
     int io_buffer_index;
-- 
1.7.11.7

Re: [Qemu-devel] [PATCH v3 1/1] atapi: make change media detection for guests easier

[Kevin Wolf]

Am 21.11.2012 18:17, schrieb Pavel Hrdina:
> If you have a guest with a media in the optical drive and you change the media
> the windows guest cannot properly recognize this media change.
> 
> Windows needs to detect the sense "NOT_READY with ASC_MEDIUM_NOT_PRESENT"
> before we send the sense "UNIT_ATTENTION with ASC_MEDIUM_MAY_HAVE_CHANGED".
> 
> v3: remove timeout as it isn't needed anymore
> 
> v2: disable debug messages
> 
> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>

Hope that we'll get libqos soon so that IDE can be properly tested with
qtest. CD-ROM media change is something that broke more than once.

The change makes sense to me, it's one of these "how could this ever
work?" cases. However I do have one or two comments:

> ---
>  hw/ide/atapi.c    | 16 +++++++++++-----
>  hw/ide/core.c     | 12 ++++++++++++
>  hw/ide/internal.h |  1 +
>  3 files changed, 24 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/ide/atapi.c b/hw/ide/atapi.c
> index 685cbaa..1534afe 100644
> --- a/hw/ide/atapi.c
> +++ b/hw/ide/atapi.c
> @@ -1124,12 +1124,18 @@ void ide_atapi_cmd(IDEState *s)
>       * GET_EVENT_STATUS_NOTIFICATION to detect such tray open/close
>       * states rely on this behavior.
>       */
> -    if (!s->tray_open && bdrv_is_inserted(s->bs) && s->cdrom_changed) {
> -        ide_atapi_cmd_error(s, NOT_READY, ASC_MEDIUM_NOT_PRESENT);
> +    if (!(atapi_cmd_table[s->io_buffer[0]].flags & ALLOW_UA) &&
> +        !s->tray_open && bdrv_is_inserted(s->bs) && s->cdrom_changed) {
> +
> +        if (!s->fake_cdrom_eject) {
> +            ide_atapi_cmd_error(s, NOT_READY, ASC_MEDIUM_NOT_PRESENT);
> +            s->fake_cdrom_eject = 1;
> +        } else {
> +            ide_atapi_cmd_error(s, UNIT_ATTENTION, ASC_MEDIUM_MAY_HAVE_CHANGED);
> +            s->fake_cdrom_eject = 0;
> +            s->cdrom_changed = 0;
> +        }
>  
> -        s->cdrom_changed = 0;
> -        s->sense_key = UNIT_ATTENTION;
> -        s->asc = ASC_MEDIUM_MAY_HAVE_CHANGED;
>          return;
>      }
>  
> diff --git a/hw/ide/core.c b/hw/ide/core.c
> index 7d6b0fa..013671a 100644
> --- a/hw/ide/core.c
> +++ b/hw/ide/core.c
> @@ -1851,6 +1851,7 @@ static void ide_reset(IDEState *s)
>      s->sense_key = 0;
>      s->asc = 0;
>      s->cdrom_changed = 0;
> +    s->fake_cdrom_eject = 0;
>      s->packet_transfer_size = 0;
>      s->elementary_transfer_size = 0;
>      s->io_buffer_index = 0;
> @@ -2143,6 +2144,16 @@ static int transfer_end_table_idx(EndTransferFunc *fn)
>      return -1;
>  }
>  
> +static void ide_drive_pre_save(void *opaque)
> +{
> +    IDEState *s = opaque;
> +
> +    if (s->cdrom_changed) {
> +        s->sense_key = UNIT_ATTENTION;
> +        s->asc = ASC_MEDIUM_MAY_HAVE_CHANGED;
> +    }
> +}

If we migrate immediately after the media change, before the OS has sent
another request, we skip the ASC_MEDIUM_NOT_PRESENT step, don't we? As
far as I can tell, adding this step is the real fix, so won't media
change break during migration this way?

The other thing is that if it's valid to set s->sense_key/asc in any
place instead of just during the start of a command (is it? I would
guess so, but I'm not sure), why don't we set ASC_MEDIUM_NOT_PRESENT
already in the change handler?

Another thing I would consider is using cdrom_changed = 0/1/2 instead of
adding fake_cdrom_eject to add another state. Migration would
automatically do the right thing for it, old versions would in both 1
and 2 state switch to ASC_MEDIUM_MAY_HAVE_CHANGED next, which is correct
in the latter case and is in the former case the same bug as the old
qemu we're migrating to has anyway.

Kevin

Re: [Qemu-devel] [PATCH v3 1/1] atapi: make change media detection for guests easier

[Pavel Hrdina]

On Fri, 2012-11-23 at 13:09 +0100, Kevin Wolf wrote:
> Am 21.11.2012 18:17, schrieb Pavel Hrdina:
> > If you have a guest with a media in the optical drive and you change the media
> > the windows guest cannot properly recognize this media change.
> > 
> > Windows needs to detect the sense "NOT_READY with ASC_MEDIUM_NOT_PRESENT"
> > before we send the sense "UNIT_ATTENTION with ASC_MEDIUM_MAY_HAVE_CHANGED".
> > 
> > v3: remove timeout as it isn't needed anymore
> > 
> > v2: disable debug messages
> > 
> > Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> 
> Hope that we'll get libqos soon so that IDE can be properly tested with
> qtest. CD-ROM media change is something that broke more than once.
> 
> The change makes sense to me, it's one of these "how could this ever
> work?" cases. However I do have one or two comments:
> 
> > ---
> >  hw/ide/atapi.c    | 16 +++++++++++-----
> >  hw/ide/core.c     | 12 ++++++++++++
> >  hw/ide/internal.h |  1 +
> >  3 files changed, 24 insertions(+), 5 deletions(-)
> > 
> > diff --git a/hw/ide/atapi.c b/hw/ide/atapi.c
> > index 685cbaa..1534afe 100644
> > --- a/hw/ide/atapi.c
> > +++ b/hw/ide/atapi.c
> > @@ -1124,12 +1124,18 @@ void ide_atapi_cmd(IDEState *s)
> >       * GET_EVENT_STATUS_NOTIFICATION to detect such tray open/close
> >       * states rely on this behavior.
> >       */
> > -    if (!s->tray_open && bdrv_is_inserted(s->bs) && s->cdrom_changed) {
> > -        ide_atapi_cmd_error(s, NOT_READY, ASC_MEDIUM_NOT_PRESENT);
> > +    if (!(atapi_cmd_table[s->io_buffer[0]].flags & ALLOW_UA) &&
> > +        !s->tray_open && bdrv_is_inserted(s->bs) && s->cdrom_changed) {
> > +
> > +        if (!s->fake_cdrom_eject) {
> > +            ide_atapi_cmd_error(s, NOT_READY, ASC_MEDIUM_NOT_PRESENT);
> > +            s->fake_cdrom_eject = 1;
> > +        } else {
> > +            ide_atapi_cmd_error(s, UNIT_ATTENTION, ASC_MEDIUM_MAY_HAVE_CHANGED);
> > +            s->fake_cdrom_eject = 0;
> > +            s->cdrom_changed = 0;
> > +        }
> >  
> > -        s->cdrom_changed = 0;
> > -        s->sense_key = UNIT_ATTENTION;
> > -        s->asc = ASC_MEDIUM_MAY_HAVE_CHANGED;
> >          return;
> >      }
> >  
> > diff --git a/hw/ide/core.c b/hw/ide/core.c
> > index 7d6b0fa..013671a 100644
> > --- a/hw/ide/core.c
> > +++ b/hw/ide/core.c
> > @@ -1851,6 +1851,7 @@ static void ide_reset(IDEState *s)
> >      s->sense_key = 0;
> >      s->asc = 0;
> >      s->cdrom_changed = 0;
> > +    s->fake_cdrom_eject = 0;
> >      s->packet_transfer_size = 0;
> >      s->elementary_transfer_size = 0;
> >      s->io_buffer_index = 0;
> > @@ -2143,6 +2144,16 @@ static int transfer_end_table_idx(EndTransferFunc *fn)
> >      return -1;
> >  }
> >  
> > +static void ide_drive_pre_save(void *opaque)
> > +{
> > +    IDEState *s = opaque;
> > +
> > +    if (s->cdrom_changed) {
> > +        s->sense_key = UNIT_ATTENTION;
> > +        s->asc = ASC_MEDIUM_MAY_HAVE_CHANGED;
> > +    }
> > +}
> 
> If we migrate immediately after the media change, before the OS has sent
> another request, we skip the ASC_MEDIUM_NOT_PRESENT step, don't we? As
> far as I can tell, adding this step is the real fix, so won't media
> change break during migration this way?
> 

We do not skip the ASC_MEDIUM_NOT_PRESENT. If we migrate immediately
after the media change, then in ide_drive_pre_save we set
ASC_MEDIUM_MAY_HAVE_CHANGED but on the other side in function
ide_drive_post_load we check if ASC_MEDIUM_MAY_HAVE_CHANGED is set and
if it is we set cdrom_changed to 1 but fake_cdrom_eject is 0. That means
the ASC_MEDIUM_NOT_PRESENT will be returned before
ASC_MEDIUM_MAY_HAVE_CHANGED.

> The other thing is that if it's valid to set s->sense_key/asc in any
> place instead of just during the start of a command (is it? I would
> guess so, but I'm not sure), why don't we set ASC_MEDIUM_NOT_PRESENT
> already in the change handler?
> 

Well, we can set it in any place, but we have to call
ide_atapi_cmd_error to let the guest know about this sense_key/asc only
as response to command request.

> Another thing I would consider is using cdrom_changed = 0/1/2 instead of
> adding fake_cdrom_eject to add another state. Migration would
> automatically do the right thing for it, old versions would in both 1
> and 2 state switch to ASC_MEDIUM_MAY_HAVE_CHANGED next, which is correct
> in the latter case and is in the former case the same bug as the old
> qemu we're migrating to has anyway.
> 

I do it that way at first, but then I rewrite it, because I thought that
using new state would be better. But if you agree with cdrom_changed =
0/1/2, I'll change it.

Pavel

> Kevin

Re: [Qemu-devel] [PATCH v3 1/1] atapi: make change media detection for guests easier

[Kevin Wolf]

Am 26.11.2012 13:43, schrieb Pavel Hrdina:
> On Fri, 2012-11-23 at 13:09 +0100, Kevin Wolf wrote:
>> Am 21.11.2012 18:17, schrieb Pavel Hrdina:
>>> diff --git a/hw/ide/core.c b/hw/ide/core.c
>>> index 7d6b0fa..013671a 100644
>>> --- a/hw/ide/core.c
>>> +++ b/hw/ide/core.c
>>> @@ -1851,6 +1851,7 @@ static void ide_reset(IDEState *s)
>>>      s->sense_key = 0;
>>>      s->asc = 0;
>>>      s->cdrom_changed = 0;
>>> +    s->fake_cdrom_eject = 0;
>>>      s->packet_transfer_size = 0;
>>>      s->elementary_transfer_size = 0;
>>>      s->io_buffer_index = 0;
>>> @@ -2143,6 +2144,16 @@ static int transfer_end_table_idx(EndTransferFunc *fn)
>>>      return -1;
>>>  }
>>>  
>>> +static void ide_drive_pre_save(void *opaque)
>>> +{
>>> +    IDEState *s = opaque;
>>> +
>>> +    if (s->cdrom_changed) {
>>> +        s->sense_key = UNIT_ATTENTION;
>>> +        s->asc = ASC_MEDIUM_MAY_HAVE_CHANGED;
>>> +    }
>>> +}
>>
>> If we migrate immediately after the media change, before the OS has sent
>> another request, we skip the ASC_MEDIUM_NOT_PRESENT step, don't we? As
>> far as I can tell, adding this step is the real fix, so won't media
>> change break during migration this way?
>>
> 
> We do not skip the ASC_MEDIUM_NOT_PRESENT. If we migrate immediately
> after the media change, then in ide_drive_pre_save we set
> ASC_MEDIUM_MAY_HAVE_CHANGED but on the other side in function
> ide_drive_post_load we check if ASC_MEDIUM_MAY_HAVE_CHANGED is set and
> if it is we set cdrom_changed to 1 but fake_cdrom_eject is 0. That means
> the ASC_MEDIUM_NOT_PRESENT will be returned before
> ASC_MEDIUM_MAY_HAVE_CHANGED.

Hm, yes, I missed the existing ide_drive_post_load() implementation.
However, this is what the code looks like:

    if (version_id < 3) {
        if (s->sense_key == UNIT_ATTENTION &&
            s->asc == ASC_MEDIUM_MAY_HAVE_CHANGED) {
            s->cdrom_changed = 1;
        }
    }

version_id for current qemu version is 3, so the intended magic doesn't
happen.

>> The other thing is that if it's valid to set s->sense_key/asc in any
>> place instead of just during the start of a command (is it? I would
>> guess so, but I'm not sure), why don't we set ASC_MEDIUM_NOT_PRESENT
>> already in the change handler?
>>
> 
> Well, we can set it in any place, but we have to call
> ide_atapi_cmd_error to let the guest know about this sense_key/asc only
> as response to command request.

Considering that the goal isn't really to set sense_key/asc so that the
guest can read it, but just so s->cdrom_changed is right on the
destination, I agree that it makes sense as it is.

>> Another thing I would consider is using cdrom_changed = 0/1/2 instead of
>> adding fake_cdrom_eject to add another state. Migration would
>> automatically do the right thing for it, old versions would in both 1
>> and 2 state switch to ASC_MEDIUM_MAY_HAVE_CHANGED next, which is correct
>> in the latter case and is in the former case the same bug as the old
>> qemu we're migrating to has anyway.
>>
> 
> I do it that way at first, but then I rewrite it, because I thought that
> using new state would be better. But if you agree with cdrom_changed =
> 0/1/2, I'll change it.

I think it's nicer to have only one state. And if I'm not mistaken, we
can even do without the pre_save/post_load handlers then.

Kevin

Re: [Qemu-devel] [PATCH v3 1/1] atapi: make change media detection for guests easier

[Pavel Hrdina]

On Mon, 2012-11-26 at 14:55 +0100, Kevin Wolf wrote:
> Am 26.11.2012 13:43, schrieb Pavel Hrdina:
> > On Fri, 2012-11-23 at 13:09 +0100, Kevin Wolf wrote:
> >> Am 21.11.2012 18:17, schrieb Pavel Hrdina:
> >>> diff --git a/hw/ide/core.c b/hw/ide/core.c
> >>> index 7d6b0fa..013671a 100644
> >>> --- a/hw/ide/core.c
> >>> +++ b/hw/ide/core.c
> >>> @@ -1851,6 +1851,7 @@ static void ide_reset(IDEState *s)
> >>>      s->sense_key = 0;
> >>>      s->asc = 0;
> >>>      s->cdrom_changed = 0;
> >>> +    s->fake_cdrom_eject = 0;
> >>>      s->packet_transfer_size = 0;
> >>>      s->elementary_transfer_size = 0;
> >>>      s->io_buffer_index = 0;
> >>> @@ -2143,6 +2144,16 @@ static int transfer_end_table_idx(EndTransferFunc *fn)
> >>>      return -1;
> >>>  }
> >>>  
> >>> +static void ide_drive_pre_save(void *opaque)
> >>> +{
> >>> +    IDEState *s = opaque;
> >>> +
> >>> +    if (s->cdrom_changed) {
> >>> +        s->sense_key = UNIT_ATTENTION;
> >>> +        s->asc = ASC_MEDIUM_MAY_HAVE_CHANGED;
> >>> +    }
> >>> +}
> >>
> >> If we migrate immediately after the media change, before the OS has sent
> >> another request, we skip the ASC_MEDIUM_NOT_PRESENT step, don't we? As
> >> far as I can tell, adding this step is the real fix, so won't media
> >> change break during migration this way?
> >>
> > 
> > We do not skip the ASC_MEDIUM_NOT_PRESENT. If we migrate immediately
> > after the media change, then in ide_drive_pre_save we set
> > ASC_MEDIUM_MAY_HAVE_CHANGED but on the other side in function
> > ide_drive_post_load we check if ASC_MEDIUM_MAY_HAVE_CHANGED is set and
> > if it is we set cdrom_changed to 1 but fake_cdrom_eject is 0. That means
> > the ASC_MEDIUM_NOT_PRESENT will be returned before
> > ASC_MEDIUM_MAY_HAVE_CHANGED.
> 
> Hm, yes, I missed the existing ide_drive_post_load() implementation.
> However, this is what the code looks like:
> 
>     if (version_id < 3) {
>         if (s->sense_key == UNIT_ATTENTION &&
>             s->asc == ASC_MEDIUM_MAY_HAVE_CHANGED) {
>             s->cdrom_changed = 1;
>         }
>     }
> 
> version_id for current qemu version is 3, so the intended magic doesn't
> happen.
> 
> >> The other thing is that if it's valid to set s->sense_key/asc in any
> >> place instead of just during the start of a command (is it? I would
> >> guess so, but I'm not sure), why don't we set ASC_MEDIUM_NOT_PRESENT
> >> already in the change handler?
> >>
> > 
> > Well, we can set it in any place, but we have to call
> > ide_atapi_cmd_error to let the guest know about this sense_key/asc only
> > as response to command request.
> 
> Considering that the goal isn't really to set sense_key/asc so that the
> guest can read it, but just so s->cdrom_changed is right on the
> destination, I agree that it makes sense as it is.
> 
> >> Another thing I would consider is using cdrom_changed = 0/1/2 instead of
> >> adding fake_cdrom_eject to add another state. Migration would
> >> automatically do the right thing for it, old versions would in both 1
> >> and 2 state switch to ASC_MEDIUM_MAY_HAVE_CHANGED next, which is correct
> >> in the latter case and is in the former case the same bug as the old
> >> qemu we're migrating to has anyway.
> >>
> > 
> > I do it that way at first, but then I rewrite it, because I thought that
> > using new state would be better. But if you agree with cdrom_changed =
> > 0/1/2, I'll change it.
> 
> I think it's nicer to have only one state. And if I'm not mistaken, we
> can even do without the pre_save/post_load handlers then.
> 
> Kevin

I don't think that we can do it without the pre_save/post_load handlers,
because if the cdrom_changed could be set also to 2 then in case, that
you migrate immediately after the change from "buggy" qemu to the
"fixed" qemu we have in cdrom_changed state 1, but we need state 2. And
otherwise, if you want to migrate from "fixed" qemu to "buggy" qemu
where version_id is lower then 3, you have to set the asc to
ASC_MEDIUM_MAY_HAVE_CHANGED.

I'll make a new version of this patch and then we can discus about it.

Pavel

Re: [Qemu-devel] [PATCH v3 1/1] atapi: make change media detection for guests easier

[Kevin Wolf]

Am 26.11.2012 15:56, schrieb Pavel Hrdina:
> On Mon, 2012-11-26 at 14:55 +0100, Kevin Wolf wrote:
>> Am 26.11.2012 13:43, schrieb Pavel Hrdina:
>>> On Fri, 2012-11-23 at 13:09 +0100, Kevin Wolf wrote:
>>>> Another thing I would consider is using cdrom_changed = 0/1/2 instead of
>>>> adding fake_cdrom_eject to add another state. Migration would
>>>> automatically do the right thing for it, old versions would in both 1
>>>> and 2 state switch to ASC_MEDIUM_MAY_HAVE_CHANGED next, which is correct
>>>> in the latter case and is in the former case the same bug as the old
>>>> qemu we're migrating to has anyway.
>>>>
>>>
>>> I do it that way at first, but then I rewrite it, because I thought that
>>> using new state would be better. But if you agree with cdrom_changed =
>>> 0/1/2, I'll change it.
>>
>> I think it's nicer to have only one state. And if I'm not mistaken, we
>> can even do without the pre_save/post_load handlers then.
>>
>> Kevin
> 
> I don't think that we can do it without the pre_save/post_load handlers,
> because if the cdrom_changed could be set also to 2 then in case, that
> you migrate immediately after the change from "buggy" qemu to the
> "fixed" qemu we have in cdrom_changed state 1, but we need state 2. 

But you can't tell which is the right state anyway, because the buggy
qemu on your source has only one "changed" state. Choosing state 1 seems
fine to me. The worst thing that can happen is that you return
ASC_MEDIUM_NOT_PRESENT twice - not a big deal.

> And
> otherwise, if you want to migrate from "fixed" qemu to "buggy" qemu
> where version_id is lower then 3, you have to set the asc to
> ASC_MEDIUM_MAY_HAVE_CHANGED.

This part is true. However, I'm pretty sure that migration from 1.3 to
0.11 doesn't work anyway, so why bother?

Kevin

Re: [Qemu-devel] [PATCH v3 1/1] atapi: make change media detection for guests easier

[Pavel Hrdina]

On Mon, 2012-11-26 at 16:08 +0100, Kevin Wolf wrote:
> Am 26.11.2012 15:56, schrieb Pavel Hrdina:
> > On Mon, 2012-11-26 at 14:55 +0100, Kevin Wolf wrote:
> >> Am 26.11.2012 13:43, schrieb Pavel Hrdina:
> >>> On Fri, 2012-11-23 at 13:09 +0100, Kevin Wolf wrote:
> >>>> Another thing I would consider is using cdrom_changed = 0/1/2 instead of
> >>>> adding fake_cdrom_eject to add another state. Migration would
> >>>> automatically do the right thing for it, old versions would in both 1
> >>>> and 2 state switch to ASC_MEDIUM_MAY_HAVE_CHANGED next, which is correct
> >>>> in the latter case and is in the former case the same bug as the old
> >>>> qemu we're migrating to has anyway.
> >>>>
> >>>
> >>> I do it that way at first, but then I rewrite it, because I thought that
> >>> using new state would be better. But if you agree with cdrom_changed =
> >>> 0/1/2, I'll change it.
> >>
> >> I think it's nicer to have only one state. And if I'm not mistaken, we
> >> can even do without the pre_save/post_load handlers then.
> >>
> >> Kevin
> > 
> > I don't think that we can do it without the pre_save/post_load handlers,
> > because if the cdrom_changed could be set also to 2 then in case, that
> > you migrate immediately after the change from "buggy" qemu to the
> > "fixed" qemu we have in cdrom_changed state 1, but we need state 2. 
> 
> But you can't tell which is the right state anyway, because the buggy
> qemu on your source has only one "changed" state. Choosing state 1 seems
> fine to me. The worst thing that can happen is that you return
> ASC_MEDIUM_NOT_PRESENT twice - not a big deal.

At first I wanted to make state 1 as ASC_MEDIUM_MAY_HAVE_CHANGED and
state 2 as ASC_MEDIUM_NOT_PRESENT. In that case I would have to in
post_load and in ide_cd_change_cb set cdrom_changed to 2.

But if we switch these states, then only what we have to do is fix the
code in atapi.c. I'll sent the patch.

> 
> > And
> > otherwise, if you want to migrate from "fixed" qemu to "buggy" qemu
> > where version_id is lower then 3, you have to set the asc to
> > ASC_MEDIUM_MAY_HAVE_CHANGED.
> 
> This part is true. However, I'm pretty sure that migration from 1.3 to
> 0.11 doesn't work anyway, so why bother?

Yes, you're right.

Pavel

> 
> Kevin
>