From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:53622) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UVQkm-0005oZ-2I for qemu-devel@nongnu.org; Thu, 25 Apr 2013 14:18:48 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UVQkg-000107-0k for qemu-devel@nongnu.org; Thu, 25 Apr 2013 14:18:43 -0400 Received: from mx1.redhat.com ([209.132.183.28]:8147) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UVQkf-0000zy-Q7 for qemu-devel@nongnu.org; Thu, 25 Apr 2013 14:18:37 -0400 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r3PIIb9c004976 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Thu, 25 Apr 2013 14:18:37 -0400 From: Markus Armbruster References: <1366393639-20651-1-git-send-email-lcapitulino@redhat.com> <1366393639-20651-3-git-send-email-lcapitulino@redhat.com> Date: Thu, 25 Apr 2013 20:18:35 +0200 In-Reply-To: <1366393639-20651-3-git-send-email-lcapitulino@redhat.com> (Luiz Capitulino's message of "Fri, 19 Apr 2013 13:47:19 -0400") Message-ID: <87mwsmlbic.fsf@blackfin.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain 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: Luiz Capitulino Cc: kwolf@redhat.com, phrdina@redhat.com, qemu-devel@nongnu.org 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. 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.