From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:42496) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UVTWU-0000ff-Qe for qemu-devel@nongnu.org; Thu, 25 Apr 2013 17:16:12 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UVTWT-00020m-H2 for qemu-devel@nongnu.org; Thu, 25 Apr 2013 17:16:10 -0400 Received: from mx1.redhat.com ([209.132.183.28]:44516) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UVTWT-00020g-8V for qemu-devel@nongnu.org; Thu, 25 Apr 2013 17:16:09 -0400 Received: from int-mx02.intmail.prod.int.phx2.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r3PLG8b8029460 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Thu, 25 Apr 2013 17:16:08 -0400 Date: Thu, 25 Apr 2013 17:16:07 -0400 From: Luiz Capitulino Message-ID: <20130425171607.7ae52bab@redhat.com> In-Reply-To: <87mwsmlbic.fsf@blackfin.pond.sub.org> References: <1366393639-20651-1-git-send-email-lcapitulino@redhat.com> <1366393639-20651-3-git-send-email-lcapitulino@redhat.com> <87mwsmlbic.fsf@blackfin.pond.sub.org> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 2/2] block: move bdrv_dev_change_media_cb() to callers that really need it List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: kwolf@redhat.com, phrdina@redhat.com, qemu-devel@nongnu.org On Thu, 25 Apr 2013 20:18:35 +0200 Markus Armbruster wrote: > Luiz Capitulino writes: > > > Commit 9ca111544c64b5abed2e79cf52e19a8f227b347b moved the call to > > bdrv_dev_change_media_cb() outside the media check in bdrv_close(), > > this added a regression where spurious DEVICE_TRAY_MOVED events > > are emitted at shutdown. > > > > To fix that this commit moves the bdrv_dev_change_media_cb() calls > > to the callers that really need to report a media change, which > > are eject_device() and do_drive_del(). This fixes the problem > > commit 9ca1115 intended to fix, plus the spurious events. > > > > Signed-off-by: Luiz Capitulino > > --- > > block.c | 2 -- > > blockdev.c | 2 ++ > > 2 files changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/block.c b/block.c > > index 90d0ed1..7fc3014 100644 > > --- a/block.c > > +++ b/block.c > > @@ -1342,8 +1342,6 @@ void bdrv_close(BlockDriverState *bs) > > } > > } > > > > - bdrv_dev_change_media_cb(bs, false); > > - > > /*throttling disk I/O limits*/ > > if (bs->io_limits_enabled) { > > bdrv_io_limits_disable(bs); > > diff --git a/blockdev.c b/blockdev.c > > index 8a1652b..f1f3b6e 100644 > > --- a/blockdev.c > > +++ b/blockdev.c > > @@ -950,6 +950,7 @@ static void eject_device(BlockDriverState *bs, int force, Error **errp) > > } > > > > bdrv_close(bs); > > + bdrv_dev_change_media_cb(bs, false); > > } > > > > void qmp_eject(const char *device, bool has_force, bool force, Error **errp) > > @@ -1100,6 +1101,7 @@ int do_drive_del(Monitor *mon, const QDict *qdict, QObject **ret_data) > > bdrv_drain_all(); > > bdrv_flush(bs); > > bdrv_close(bs); > > + bdrv_dev_change_media_cb(bs, false); > > > > /* if we have a device attached to this BlockDriverState > > * then we need to make the drive anonymous until the device > > Invariant: callback does nothing unless a device model with removable > media is connected (dev_ops->change_media_cb set). > > Before 9ca1115: Callback runs on any bdrv_close() that actually ejects a > medium. > > Since 9ca1115: Callback runs on any bdrv_close() > > With this patch applied: Callback runs in eject_device() and > do_drive_del(). No change, except it now runs after > bdrv_io_limits_disable(), which shouldn't matter. This is the trivial > part of the review. > > Now the non-trivial part. Callback no longer runs in > > * bdrv_open() when it fails because it can't open the backing file > > * bdrv_open() when it fails because the block driver doesn't consume the > all options > > * bdrv_delete() > - bdrv_file_open() error path > - bdrv_open_backing_file() error path > - bdrv_open() snapshot=on path > - bdrv_open(), purpose not obvious, perhaps related to format probing > - bdrv_open() some error paths > - bdrv_close(), bs->backing_hd > - bdrv_close(), bs->file > - bdrv_drop_intermediate() > - bdrv_snapshot_goto() error path > - bdrv_img_create() > - drive_uninit() > - drive_init() error path > - qmp_transaction() error path > - qmp_drive_mirror() some error paths > - qemu_img.c many places > - qemu_io.c many places > - blkverify_open() error path > - blkverify_close() > - cow_create() > - mirror_run() > - qcow_create() > - qcow2_create2() > - qed_create() > - sheepdog.c's sd_prealloc(), sd_create() > - close_unused_images() > - vmdk_free_extents(), vmdk_parse_extents(), vmdk_create() > - vvfat.c's write_target_close() > > * qemu-nbd.c's main() > > * mirror_run() > > * qcow2_create2() > > * pci_piix3_xen_ide_unplug() > > * bdrv_close_all() > - qemu-nbd.c, from atexit() Same as above. > - vl.c's main(). > > For each of them, I'd like to see an argument why the not running the > callback is okay. Very encouraging :( I wonder if such a comprehensive analysis was done when 1 adding the callback in the first place and 2 on 9ca1115. > A good one is "no device model can be attached", say > because the BDS isn't a root (device models only attach to roots), or > because the BDS hasn't escaped its constructor, yet. > > Not wanting to do all this work is exactly why I refrained from > attempting to fix the problem commit 9ca1115 attempts to fix (I > suggested to declare it a feature instead). Didn't help, because I also > refrained from NAKing that fix, and here we are. > > I'll do what I can, as time permits, but help is certainly appreciated. The bug I'm fixing is not a serious one, but this makes me think that reverting 9ca1115 is the best thing to do.