From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([140.186.70.92]:42718)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <lcapitulino@redhat.com>) id 1QjwYT-0001Hb-66
	for qemu-devel@nongnu.org; Thu, 21 Jul 2011 12:56:58 -0400
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <lcapitulino@redhat.com>) id 1QjwYO-0006i1-9g
	for qemu-devel@nongnu.org; Thu, 21 Jul 2011 12:56:57 -0400
Received: from mx1.redhat.com ([209.132.183.28]:51649)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <lcapitulino@redhat.com>) id 1QjwYN-0006hq-Se
	for qemu-devel@nongnu.org; Thu, 21 Jul 2011 12:56:52 -0400
Date: Thu, 21 Jul 2011 13:34:34 -0300
From: Luiz Capitulino <lcapitulino@redhat.com>
Message-ID: <20110721133434.3051308c@doriath>
In-Reply-To: <m362mvekpm.fsf@blackfin.pond.sub.org>
References: <1311179069-27882-1-git-send-email-armbru@redhat.com>
	<1311179069-27882-8-git-send-email-armbru@redhat.com>
	<20110721111134.787a83af@doriath>
	<m362mvekpm.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 07/55] block: Make BlockDriver method
 bdrv_set_locked() return void
List-Id: <qemu-devel.nongnu.org>
List-Unsubscribe: <https://lists.nongnu.org/mailman/options/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <https://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: Markus Armbruster <armbru@redhat.com>
Cc: kwolf@redhat.com, stefano.stabellini@eu.citrix.com, dbaryshkov@gmail.com, quintela@redhat.com, qemu-devel@nongnu.org, amit.shah@redhat.com

On Thu, 21 Jul 2011 17:07:17 +0200
Markus Armbruster <armbru@redhat.com> wrote:

> Luiz Capitulino <lcapitulino@redhat.com> writes:
> 
> > On Wed, 20 Jul 2011 18:23:41 +0200
> > Markus Armbruster <armbru@redhat.com> 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.

I can't tell if the problems you mention above (physical lock out of sync
with the virtual one and the post migration) will turn out to be real
issues. If they do, we'll have to fix it.