From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:38986) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UopN4-0006DB-4d for qemu-devel@nongnu.org; Tue, 18 Jun 2013 02:26:27 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UopN1-0000gW-Hd for qemu-devel@nongnu.org; Tue, 18 Jun 2013 02:26:26 -0400 Received: from mx1.redhat.com ([209.132.183.28]:10170) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UopN1-0000gS-8O for qemu-devel@nongnu.org; Tue, 18 Jun 2013 02:26:23 -0400 Message-ID: <51BFFD8B.1060503@redhat.com> Date: Tue, 18 Jun 2013 08:26:19 +0200 From: Pavel Hrdina MIME-Version: 1.0 References: <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> <20130617105936.6262611c@redhat.com> <20130617151642.GG3994@dhcp-200-207.str.redhat.com> In-Reply-To: <20130617151642.GG3994@dhcp-200-207.str.redhat.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed 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 , qemu-devel@nongnu.org, armbru@redhat.com, Luiz Capitulino On 17.6.2013 17:16, Kevin Wolf wrote: > Am 17.06.2013 um 16:59 hat Luiz Capitulino geschrieben: >> 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. > > Sure, I agree with one patch per logical change. > > Kevin > In that case the v3 series is ready for apply? I will write another patch to fix this bug. Pavel