From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:35838) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YTAkZ-00074L-1y for qemu-devel@nongnu.org; Wed, 04 Mar 2015 09:58:16 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YTAkU-0007sk-2K for qemu-devel@nongnu.org; Wed, 04 Mar 2015 09:58:14 -0500 Received: from mx1.redhat.com ([209.132.183.28]:46584) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YTAkT-0007sN-R5 for qemu-devel@nongnu.org; Wed, 04 Mar 2015 09:58:09 -0500 Received: from int-mx13.intmail.prod.int.phx2.redhat.com (int-mx13.intmail.prod.int.phx2.redhat.com [10.5.11.26]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id t24Ew8Es013023 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL) for ; Wed, 4 Mar 2015 09:58:09 -0500 Message-ID: <54F71D7E.7050300@redhat.com> Date: Wed, 04 Mar 2015 09:58:06 -0500 From: Max Reitz MIME-Version: 1.0 References: <1423501897-30410-1-git-send-email-mreitz@redhat.com> <1423501897-30410-5-git-send-email-mreitz@redhat.com> <20150304140242.GQ3465@noname.str.redhat.com> <54F711AB.1060703@redhat.com> <20150304142037.GT3465@noname.str.redhat.com> <54F71590.3030502@redhat.com> <20150304143920.GU3465@noname.str.redhat.com> <54F719B3.9020203@redhat.com> <20150304145316.GV3465@noname.str.redhat.com> In-Reply-To: <20150304145316.GV3465@noname.str.redhat.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v2 04/37] hw/usb-storage: Check whether BB is inserted List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf Cc: John Snow , qemu-devel@nongnu.org, Stefan Hajnoczi , Markus Armbruster 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 >>>>>>>> Reviewed-by: Eric Blake >>>>>>>> --- >>>>>>>> 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