Re: Poking around bdrv_is_inserted()

[Kevin Wolf]

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. 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.

> >         }
> >         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.

Kevin