Re: Poking around bdrv_is_inserted()

[Markus Armbruster <armbru@redhat.com>]

Kevin Wolf <kwolf@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 <armbru@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?