From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:55522) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YTADm-0002Q9-Ua for qemu-devel@nongnu.org; Wed, 04 Mar 2015 09:24:26 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YTADj-0001aF-BL for qemu-devel@nongnu.org; Wed, 04 Mar 2015 09:24:22 -0500 Received: from mx1.redhat.com ([209.132.183.28]:36048) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YTADj-0001a3-3N for qemu-devel@nongnu.org; Wed, 04 Mar 2015 09:24:19 -0500 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id t24EOIc5013070 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL) for ; Wed, 4 Mar 2015 09:24:18 -0500 Message-ID: <54F71590.3030502@redhat.com> Date: Wed, 04 Mar 2015 09:24:16 -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> In-Reply-To: <20150304142037.GT3465@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: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. Yes, I will include my patch order justification chapter from v1 in the cover letter of v3. Max