From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43)
	id 1MWCfk-0002q8-5U
	for qemu-devel@nongnu.org; Wed, 29 Jul 2009 13:10:36 -0400
Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43)
	id 1MWCff-0002gi-Es
	for qemu-devel@nongnu.org; Wed, 29 Jul 2009 13:10:35 -0400
Received: from [199.232.76.173] (port=54935 helo=monty-python.gnu.org)
	by lists.gnu.org with esmtp (Exim 4.43) id 1MWCff-0002gb-17
	for qemu-devel@nongnu.org; Wed, 29 Jul 2009 13:10:31 -0400
Received: from mail-ew0-f210.google.com ([209.85.219.210]:47550)
	by monty-python.gnu.org with esmtp (Exim 4.60)
	(envelope-from <filip.navara@gmail.com>) id 1MWCfe-0007Cf-JS
	for qemu-devel@nongnu.org; Wed, 29 Jul 2009 13:10:30 -0400
Received: by ewy6 with SMTP id 6so125647ewy.34
	for <qemu-devel@nongnu.org>; Wed, 29 Jul 2009 10:10:29 -0700 (PDT)
MIME-Version: 1.0
In-Reply-To: <20090729160902.GG30449@redhat.com>
References: <20090729160902.GG30449@redhat.com>
Date: Wed, 29 Jul 2009 19:10:29 +0200
Message-ID: <5b31733c0907291010k6ce95d54g6cbaaa954f22409b@mail.gmail.com>
Subject: Re: [Qemu-devel] [PATCH v3] make windows notice media change
From: Filip Navara <filip.navara@gmail.com>
Content-Type: text/plain; charset=ISO-8859-1
Content-Transfer-Encoding: quoted-printable
List-Id: qemu-devel.nongnu.org
List-Unsubscribe: <http://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.gnu.org/pipermail/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <http://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: Gleb Natapov <gleb@redhat.com>
Cc: qemu-devel@nongnu.org

On Wed, Jul 29, 2009 at 6:09 PM, Gleb Natapov<gleb@redhat.com> wrote:
> @@ -3250,6 +3253,8 @@ static int pci_ide_load(QEMUFile* f, void *opaque, =
int version_id)
> =A0 =A0 /* per IDE drive data */
> =A0 =A0 for(i =3D 0; i < 4; i++) {
> =A0 =A0 =A0 =A0 ide_load(f, &d->ide_if[i]);
> + =A0 =A0 =A0 =A0if (version_id =3D=3D 3)
> + =A0 =A0 =A0 =A0 =A0 =A0qemu_get_8s(f, &d->ide_if[i].cdrom_changed);
> =A0 =A0 }
> =A0 =A0 return 0;
> =A0}

I'd prefer passing the version to ide_load and doing the actual load there.=
..

... but the patch is all wrong and based on wrong assumptions, which is
much more fundamental problem. Windows cdrom driver is not that stupid
about the change as you think.

The cdrom driver really has a timer and polls the IDE controller, but it
doesn't require the intermediate ASC_MEDIUM_NOT_PRESENT state
you introduced. It's perfectly ok to return SENSE_UNIT_ATTENTION /
ASC_MEDIUM_MAY_HAVE_CHANGED from GPCMD_TEST_UNIT_READY
and Windows will recognize it as medium change.

Something like this should work:
        if (bdrv_is_inserted(s->bs)) {
            if (s->cdrom_changed) {
                ide_atapi_cmd_error(s, SENSE_UNIT_ATTENTION,
                                 ASC_MEDIUM_MAY_HAVE_CHANGED);
                s->cdrom_changed =3D 0;
            } else {
                ide_atapi_cmd_ok(s);
            }
        } else {
            ide_atapi_cmd_error(s, SENSE_NOT_READY,
                                ASC_MEDIUM_NOT_PRESENT);
        }

The benefit is that it will not break guests which issue the request only
once.

Best regards,
Filip Navara