Re: Poking around bdrv_is_inserted()

Re: Poking around bdrv_is_inserted()

[Markus Armbruster]

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").
>
>         }
>         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.
>
> * 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()?

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

Re: Poking around bdrv_is_inserted()

[Markus Armbruster]

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?

Re: Poking around bdrv_is_inserted()

[Kevin Wolf]

Am 09.11.2021 um 16:20 hat Markus Armbruster geschrieben:
> 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.

It does. blk_bs(blk) returns NULL for 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.
> 
> I'm not sure having bdrv_is_inserted() return true for "broken"
> backends makes sense.

I wonder if bdrv_is_inserted() makes sense at all (why not just do
whatever you were planning to do if it returns true, and handle the
error?).

But anyway, it returns false for broken backends.

Callers might commonly not be interested in "is a medium inserted?", but
more in "can I access the medium?". In this case, returning false
provides the right answer.

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

I'm surprised to learn that host_device actually implements
.bdrv_co_pwritev.

So yes, I suppose if you have a qcow2 formatted CD with a snapshot in
it, and you insert it into your physical drive and somehow convince the
kernel to let us open it read-write, and then you eject the CD while the
guest is running and try to delete the snapshot, then you might get the
wrong error message.

I think this is still deep in "then don't do that" territory, but if you
feel like slapping a bdrv_is_inserted() on it, feel free.

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

We don't actually have checks to prevent it, but I doubt you can build
anything meaningful with the combination of removable media and non-raw
drivers.

I know qcow2 will be horribly confused if you swap out the file under
its feet. If you must, you can change bs->file (even without host_cdrom,
blockdev-reopen should be enough). If it breaks, you get to keep the
pieces.

So it probably only makes a difference in a case that can't work anyway.

Kevin