From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:56923) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1elF8D-0001Y3-Jn for qemu-devel@nongnu.org; Mon, 12 Feb 2018 09:30:58 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1elF89-0002Zo-ED for qemu-devel@nongnu.org; Mon, 12 Feb 2018 09:30:57 -0500 Date: Mon, 12 Feb 2018 15:30:40 +0100 From: Kevin Wolf Message-ID: <20180212143040.GH5103@localhost.localdomain> References: <20180207163622.29935-1-pbonzini@redhat.com> <20180207163622.29935-3-pbonzini@redhat.com> <20180208013517.GB24289@lemon.usersys.redhat.com> <5ac0b76e-213a-60c3-c010-87512d3a564a@redhat.com> <20180212135206.GG5103@localhost.localdomain> <81af35f9-4934-0c21-e3b1-b27c2fa7fdd9@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <81af35f9-4934-0c21-e3b1-b27c2fa7fdd9@redhat.com> Subject: Re: [Qemu-devel] [PATCH 2/2] scsi: add block job opblockers for scsi-block List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini Cc: Fam Zheng , qemu-devel@nongnu.org, qemu-block@nongnu.org Am 12.02.2018 um 15:00 hat Paolo Bonzini geschrieben: > On 12/02/2018 14:52, Kevin Wolf wrote: > > Am 08.02.2018 um 11:42 hat Paolo Bonzini geschrieben: > >> On 08/02/2018 02:35, Fam Zheng wrote: > >>> On Wed, 02/07 17:36, Paolo Bonzini wrote: > >>>> @@ -2626,6 +2656,36 @@ static void scsi_block_realize(SCSIDevice *dev, Error **errp) > >>>> > >>>> scsi_realize(&s->qdev, errp); > >>>> scsi_generic_read_device_identification(&s->qdev); > >>>> + > >>>> + /* For op blockers, due to lack of support for dirty bitmaps. */ > >>>> + error_setg(&sb->mirror_source, > >>>> + "scsi-block does not support acting as a mirroring source"); > >>>> + error_setg(&sb->commit_source, > >>>> + "scsi-block does not support acting as an active commit source"); > >>> > >>> An alternative way would be adding BLOCK_OP_TYPE_DIRTY_BITMAP. The error message > >>> will not be as nice but it can be useful for another (blockjob) operation that > >>> requires dirty bitmap support, or another device that doesn't support dirty > >>> bitmaps. Though there isn't one for now. > >> > >> Yeah, I thought about it. Another possibility is make BLOCK_OP_TYPE_* a > >> bitmask. Then you can easily add a single Error * for multiple > >> blockers, and BLOCK_OP_TYPE_DIRTY_BITMAP can be defined as > >> BLOCK_OP_TYPE_MIRROR_SOURCE|BLOCK_OP_TYPE_COMMIT_SOURCE; likewise for > >> notifiers below. > > > > We shouldn't be adding new instances of BLOCK_OP_* at all. I couldn't > > find the time yet to remove the existing ones, but any new protections > > should be using the permission system. > > I agree. But does this include not fixing bugs wherever clients are > using the old op blockers? I'm not saying that we shouldn't fix the bug, just that we should fix it properly with the best infrastructure we have. The old op blockers are "fixing" the problem at the symptom level, and you have to check for each high-level operation if it does something problematic internally. You have to repeat this analysis every time you add a new operation or modifiy an existing one (which noone ever does). The risk that this breaks sooner or later is pretty high. The new permission system, on the other hand, directly addresses the root cause, and any new feature that uses dirty bitmaps will then automatically get the protection, too. So in fact, I would say that the bug isn't really fixed (but at best papered over) until we add a proper fix on the permission level. Kevin