Re: [Qemu-devel] [PATCH v2 04/37] hw/usb-storage: Check whether BB is inserted

[Max Reitz]

On 2015-03-04 at 09:53, Kevin Wolf wrote:
> Am 04.03.2015 um 15:41 hat Max Reitz geschrieben:
> > On 2015-03-04 at 09:39, Kevin Wolf wrote:
> > > Am 04.03.2015 um 15:24 hat Max Reitz geschrieben:
> > > > On 2015-03-04 at 09:20, Kevin Wolf wrote:
> > > > > Am 04.03.2015 um 15:07 hat Max Reitz geschrieben:
> > > > > > On 2015-03-04 at 09:02, Kevin Wolf wrote:
> > > > > > > Am 09.02.2015 um 18:11 hat Max Reitz geschrieben:
> > > > > > > > Only call bdrv_key_required() on the BlockDriverState if the
> > > > > > > > BlockBackend has an inserted medium.
> > > > > > > >
> > > > > > > > Signed-off-by: Max Reitz <address@hidden>
> > > > > > > > Reviewed-by: Eric Blake <address@hidden>
> > > > > > > > ---
> > > > > > > >   hw/usb/dev-storage.c | 2 +-
> > > > > > > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > > > >
> > > > > > > > diff --git a/hw/usb/dev-storage.c b/hw/usb/dev-storage.c
> > > > > > > > index 4539733..3123baf 100644
> > > > > > > > --- a/hw/usb/dev-storage.c
> > > > > > > > +++ b/hw/usb/dev-storage.c
> > > > > > > > @@ -638,7 +638,7 @@ static void usb_msd_realize_storage(USBDevice *dev, Error 
> > > > > > > > **errp)
> > > > > > > >       usb_msd_handle_reset(dev);
> > > > > > > >       s->scsi_dev = scsi_dev;
> > > > > > > > -    if (bdrv_key_required(blk_bs(blk))) {
> > > > > > > > +    if (blk_is_inserted(blk) && bdrv_key_required(blk_bs(blk))) {
> > > > > > > >           if (cur_mon) {
> > > > > > > >               monitor_read_bdrv_key_start(cur_mon, blk_bs(blk),
> > > > > > > >                                           usb_msd_password_cb, s);
> > > > > > > Why would bdrv_key_required() ever return true when no medium is
> > > > > > > inserted? Sounds like a bug to me, like not resetting state correctly on
> > > > > > > bdrv_close() of an encrypted image.
> > > > > > The point is that blk_bs(blk) might be NULL.
> > > > > This is not what blk_is_inserted() is checking. It happens to protect
> > > > > you against segfaults because it's robust against using NULL, but with
> > > > > an existing BDS, checking whether there is a medium inserted (in the
> > > > > physical device for passthrough drivers) doesn't make sense.
> > > > Not right now it's not. See patch 6.
> > > Patch 6 looks unrelated, at least in v2. But if you're trying to say
> > > that I looked at the wrong version, you're right: It doesn't protect you
> > > against segfaults at this point yet (which is okay, because blk->bs
> > > can't be NULL yet), it only performs the misguided inserted check.
> > Oops, yes, I meant patch 7.
> >
> > > Doesn't answer my initial question or make that check any better.
> > The answer to your initial question is: bdrv_key_required() assumes
> > a non-NULL BDS pointer is passed (which is reasonable). Therefore,
> > it crashes when "no medium is inserted" in the sense of
> > !blk_bs(blk).
> That's a great argument in favour of checking blk_bs(bs), but I can't
> see how it's one for the completely unrelated blk_inserted(blk).

As said in IRC, I used blk_is_inserted() because it had the side-effect 
of preventing dereferencing the NULL pointer, but I like it much more 
than calling blk_bs() for that.

Why is that? Because no code outside of the block layer should access 
the BDS directly, therefore, blk_bs() should not be called outside of 
the block layer. Why don't I add blk_key_required()? Because that 
function does not work on the BB level, but only per BDS, so I will not 
add that BB function.

So I didn't want to add more of the ugliness that makes this code here 
so ugly to it, but then again, it's the only correct way, you're right 
(except for the "remove this completely out-of-place code from here").

Max