From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:52146) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Qjv3J-0001fP-LP for qemu-devel@nongnu.org; Thu, 21 Jul 2011 11:20:43 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1QjuqS-00016R-06 for qemu-devel@nongnu.org; Thu, 21 Jul 2011 11:07:24 -0400 Received: from mx1.redhat.com ([209.132.183.28]:14204) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QjuqR-00016N-Mu for qemu-devel@nongnu.org; Thu, 21 Jul 2011 11:07:23 -0400 From: Markus Armbruster References: <1311179069-27882-1-git-send-email-armbru@redhat.com> <1311179069-27882-8-git-send-email-armbru@redhat.com> <20110721111134.787a83af@doriath> Date: Thu, 21 Jul 2011 17:07:17 +0200 In-Reply-To: <20110721111134.787a83af@doriath> (Luiz Capitulino's message of "Thu, 21 Jul 2011 11:11:34 -0300") Message-ID: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Subject: Re: [Qemu-devel] [PATCH 07/55] block: Make BlockDriver method bdrv_set_locked() return void List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Luiz Capitulino Cc: kwolf@redhat.com, stefano.stabellini@eu.citrix.com, dbaryshkov@gmail.com, quintela@redhat.com, qemu-devel@nongnu.org, amit.shah@redhat.com Luiz Capitulino writes: > On Wed, 20 Jul 2011 18:23:41 +0200 > Markus Armbruster wrote: > >> The only caller is bdrv_set_locked(), and it ignores the value. >> >> Callees always return 0, except for FreeBSD's cdrom_set_locked(), >> which returns -ENOTSUP when the device is in a terminally wedged >> state. > > The fact that we're ignoring ioctl() failures caught my attention. Is it > because the state we store in bs->locked is what really matters? > > Otherwise the right thing to do would be to propagate the error and > change callers to handle it. The only caller is bdrv_set_locked(). Which can't handle it, so it would have to pass it on. The purpose of bdrv_set_locked() is to synchronize the physical lock (if any) with the virtual lock. Worst that can happen is the physical lock fails to track the virtual one. bdrv_set_locked()'s callers are device models: ide-cd and scsi-cd. Each one calls it in four places (with all my patches applied): * Device init: can return failure. Failure makes QEMU refuse to start (-device cold plug), or hot plug fail (device_add). * Device exit: can't return failure. * Device post migration: can return failure, makes migration fail. Given the choice, I figure I'd pick an incorrect physical tray lock over a failed migration. * Guest PREVENT ALLOW MEDIUM REMOVAL: can send error reply to guest. Recommended errors according to MMC-5 are "unit attention errors" (not a good match), "CDB or parameter list validation errors" (worse) and "hardware failures" (no good choices there either). I doubt adding the error handling is worth our while, but feel free to send patches.