Kevin Wolf <kw...@redhat.com> writes:
> Am 09.11.2021 um 07:44 hat Markus Armbruster geschrieben:
>> Screwed up qemu-devel@nongnu.org, sorry for the inconvenience.
>>
>> Markus Armbruster <arm...@redhat.com> writes:
>>
>> > bdrv_is_inserted() returns false when:
>> >
>> > /**
>> > * Return TRUE if the media is present
>> > */
>> > bool bdrv_is_inserted(BlockDriverState *bs)
>> > {
>> > BlockDriver *drv = bs->drv;
>> > BdrvChild *child;
>> >
>> > if (!drv) {
>> > return false;
>> >
>> > 1. @bs has no driver (this is how we represent "no medium").
>
> Not really any more. "No medium" is blk->root == NULL.
Uh, blk_is_inserted() does *not* check blk->root:
bool blk_is_inserted(BlockBackend *blk)
{
BlockDriverState *bs = blk_bs(blk);
return bs && bdrv_is_inserted(bs);
}
Now I'm confused.
> These days
> bs->drv == NULL basically means "the backend is broken". This happens
> after qcow2_signal_corruption(), and I'm not sure if we have more
> circumstances like it.
I'm not sure having bdrv_is_inserted() return true for "broken" backends
makes sense.
>> > }
>> > if (drv->bdrv_is_inserted) {
>> > return drv->bdrv_is_inserted(bs);
>> >
>> > 2. Its driver's ->bdrv_is_inserted() returns false. This is how
>> > passthrough block backends signal "host device has no medium". Right
>> > now, the only user is "host_cdrom".
>> >
>> > }
>> > QLIST_FOREACH(child, &bs->children, next) {
>> > if (!bdrv_is_inserted(child->bs)) {
>> > return false;
>> >
>> > 3. Any of its children has no medium. Common use looking through
>> > filters, which have a single child.
>> >
>> > }
>> > }
>> > return true;
>> > }
>> >
>> > Makes sense.
>> >
>> > Now look at the uses of QERR_DEVICE_HAS_NO_MEDIUM.
>> >
>> > * external_snapshot_prepare() in blockdev.c:
>> >
>> > if (!bdrv_is_inserted(state->old_bs)) {
>> > error_setg(errp, QERR_DEVICE_HAS_NO_MEDIUM, device);
>> > goto out;
>> > }
>> >
>> > where @device is the device name, i.e. BlockdevSnapshot member @node
>> > or BlockdevSnapshotSync member @device. Uh-oh: the latter can be
>> > null. If we can reach the error_setg() then, we crash on some
>> > systems.
>
> Sounds like we should write a test case and then fix it.
>
>> > * bdrv_snapshot_delete() and bdrv_snapshot_load_tmp() in
>> > block/snaphot.c:
>> >
>> > if (!drv) {
>> > error_setg(errp, QERR_DEVICE_HAS_NO_MEDIUM,
>> > bdrv_get_device_name(bs));
>> > return -ENOMEDIUM;
>> > }
>> >
>> > where @drv is bs->drv.
>> >
>> > Why do we check only for 1. here instead of calling
>> > bdrv_is_inserted()?
>
> I guess we could philosophise about the theoretically right thing to do,
> but last time I checked, host_cdrom didn't support snapshots, so it
> probably doesn't matter either way.
We could also philosophize about "any of its children has no medium".
As far as I know, nothing stops me from using a host_cdrom as a backing
file for a QCOW2, and that I *can* snapshot.
Functions (and methods) bdrv_is_inserted(), bdrv_eject(), and
bdrv_lock_medium() are related. block_int.h groups them under
/* removable device specific */, and block.c under /* removable device
support */. But only bdrv_is_inserted() recurses into children. Is
this how it should be?
