From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:35499) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UobFC-00071D-C0 for qemu-devel@nongnu.org; Mon, 17 Jun 2013 11:21:27 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UobF7-0004MA-Ih for qemu-devel@nongnu.org; Mon, 17 Jun 2013 11:21:22 -0400 Received: from mx1.redhat.com ([209.132.183.28]:42091) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UoauE-0004wY-0K for qemu-devel@nongnu.org; Mon, 17 Jun 2013 10:59:42 -0400 Date: Mon, 17 Jun 2013 10:59:36 -0400 From: Luiz Capitulino Message-ID: <20130617105936.6262611c@redhat.com> In-Reply-To: <20130617144911.GE3994@dhcp-200-207.str.redhat.com> References: <20130605132317.GF31478@stefanha-thinkpad.muc.redhat.com> <51BEDAEB.3050404@redhat.com> <20130617123310.GB30145@stefanha-thinkpad.redhat.com> <20130617092212.6cdac080@redhat.com> <51BF0E44.3060700@redhat.com> <20130617093202.2bd30f17@redhat.com> <51BF114F.3000507@redhat.com> <20130617134652.GA3994@dhcp-200-207.str.redhat.com> <20130617095131.71be4fa9@redhat.com> <20130617144911.GE3994@dhcp-200-207.str.redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v2 2/2] block: move the bdrv_dev_change_media_cb() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf Cc: Stefan Hajnoczi , Pavel Hrdina , qemu-devel@nongnu.org, armbru@redhat.com On Mon, 17 Jun 2013 16:49:11 +0200 Kevin Wolf wrote: > Am 17.06.2013 um 15:51 hat Luiz Capitulino geschrieben: > > On Mon, 17 Jun 2013 15:46:52 +0200 > > Kevin Wolf wrote: > > > > > Am 17.06.2013 um 15:38 hat Pavel Hrdina geschrieben: > > > > >>>>>It's just a warning, that you used a password for a block device that > > > > >>>>>doesn't require it. The device is opened successfully and should be > > > > >>>>>handled correctly (call the bdrv_dev_change_media_cb() ). > > > > >>>> > > > > >>>>Yep, IMO it's worth a comment that this isn't an "error" just a > > > > >>>>"warning". > > > > >>> > > > > >>>Actually, you can't have such a warning in QMP. You either fail or you > > > > >>>succeed. We should just do what the current code does. > > > > >>> > > > > >> > > > > >>This is the same logic as the old one. The device is loaded but the > > > > >>error is emitted. > > > > > > > > > >That's a bug if the operation succeeded. > > > > > > > > > > > > > In that case, how do you think, that we should handle the situation > > > > that user is trying to open device that isn't require the password, but > > > > user will provide the password? > > > > > > > > I don't think that we should fail and abort that operation. > > > > > > I think we should. The image and the options passed for it don't fit > > > together, this is an error condition. Probably the user meant to pass a > > > different image. > > > > I agree in principle, but I fear this might be an incompatible change as > > there might be clients out there assuming the VM is up and running (because > > it's what ends up happening). > > > > Thinking about this again though, the client does get an error... > > Do you think any client is sending passwords for unencrypted images? > Because if there is none (and I think we have reason to believe so), we > don't break anything if we change the behaviour. And if something > does break, we have uncovered a management tool bug, so that's not too > bad either. Yes, I agree. I was being overly cautious when I suggested dropping the error, but I think you're right: we do send an error, so a well written client should just fail and shouldn't brake if we do the right thing. So let's do the Right Thing, but I also suggest to do this in a separate commit so that it's easy to spot.