Re: [Qemu-devel] [PATCH 05/18] ide: Turn debug messages into assertions

[Markus Armbruster <armbru@redhat.com>]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 01.06.2011 17:32, schrieb Markus Armbruster:
>> Kevin Wolf <kwolf@redhat.com> writes:
>> 
>>> Am 01.06.2011 15:44, schrieb Luiz Capitulino:
>>>> On Thu, 26 May 2011 18:12:08 -0300
>>>> Luiz Capitulino <lcapitulino@redhat.com> wrote:
>>>>
>>>>> On Thu, 19 May 2011 14:33:19 +0200
>>>>> Kevin Wolf <kwolf@redhat.com> wrote:
>>>>>
>>>>>> These printfs aren't really debug messages, but clearly indicate a bug if they
>>>>>> ever become effective.
>>>>>
>>>>> Then we have a bug somewhere, starting a VM with:
>>>>>
>>>>>  # qemu -hda disks/test.img -enable-kvm -m 1G -cdrom /dev/sr0
>>>>>
>>>>> Where the host's CDROM is empty, triggers one of these asserts:
>>>>>
>>>>>  qmp-unstable/hw/ide/pci.c:299: bmdma_cmd_writeb: Assertion `bm->bus->dma->aiocb == ((void *)0)'
>>>>
>>>> I found out why this is happening. I'm passing '-snapshot' to the command-line,
>>>> sorry for not mentioning it (I forgot I was using my devel alias).
>>>
>>> And suddenly it's reproducible. :-)
>>>
>>> I'll have a look.
>>>
>>>> I also found out that /usr/bin/eject in the guest won't work when
>>>> -snapshot is used. Shouldn't qemu ignore this flag when using cdrom
>>>> passthrough?
>>>
>>> "Won't work" means that it works like with a CD-ROM image? That would be
>>> what I expect, as you end up having a qcow2 image with -snapshot.
>> 
>> With a qcow2 stacked onto the host_cdrom.  The block layer forwards the
>> guest's eject only if the top driver has a bdrv_eject() method.  Which
>> qcow2 doesn't have.  I guess bdrv_eject() fails with -ENOTSUP, and
>> cmd_start_stop_unit() reports a funny error to the guest.
>
> bdrv_eject ignores -ENOTSUP and only changes the virtual tray in this
> case. Just the very same thing as with normal ISO images.

Missed that.  Thanks.

>>> Not sure what's the best way of fixing this. Maybe just ignoring
>>> -snapshot for read-only block devices?
>> 
>> Why not, the combination is pointless.
>
> It could start making a difference in some obscure combinations. Imagine
> a read-only image with a backing file, -snapshot and the 'commit'
> monitor command.
>
> Sounds pretty insane, but I wouldn't bet that people aren't using it...

People try all kinds of insane things.  The question is whether we can
change it anyway.

>>>                                        Or we could try and forward the
>>> eject request to the backing file if the format driver doesn't handle it.
>> 
>> For what it's worth, the "raw" driver forwards manually.
>
> Yeah, but copying that into each driver probably isn't the right thing
> to do.

I didn't mean to hold up the "raw" driver as a shining example of how to
do stuff.

>        For a generic implementation we could probably first try the
> driver itself, if it returns -ENOTSUP we try the protocol, and if that
> returns -ENOTSUP, too, we ask the backing file driver.

I don't want to start another "format vs. protocol" semantic war, just
point out that the general case is a tree of block drivers, and a
general rule for passing eject up the tree better covers that.

The current block.c provides for binary trees (bs->file and
bs->backing_hd).  When we discussed blockdev_add, we came up with much
wilder trees.

> Can't wait to see the requests that with a qcow2 formatted CD with a
> backing file in another CD drive the other one (or both) should be
> ejected. :-)

Requests like that can make you wish for a way to eject users ;)