From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:58767) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UoZlF-0007Jr-4S for qemu-devel@nongnu.org; Mon, 17 Jun 2013 09:46:25 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UoZlA-00034v-94 for qemu-devel@nongnu.org; Mon, 17 Jun 2013 09:46:21 -0400 Received: from mx1.redhat.com ([209.132.183.28]:32836) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UoZlA-00034q-10 for qemu-devel@nongnu.org; Mon, 17 Jun 2013 09:46:16 -0400 Date: Mon, 17 Jun 2013 09:46:10 -0400 From: Luiz Capitulino Message-ID: <20130617094610.7b8a5b27@redhat.com> In-Reply-To: <51BF114F.3000507@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> 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: Pavel Hrdina Cc: kwolf@redhat.com, Stefan Hajnoczi , qemu-devel@nongnu.org, armbru@redhat.com On Mon, 17 Jun 2013 15:38:23 +0200 Pavel Hrdina wrote: > On 17.6.2013 15:32, Luiz Capitulino wrote: > > On Mon, 17 Jun 2013 15:25:24 +0200 > > Pavel Hrdina wrote: > > > >> On 17.6.2013 15:22, Luiz Capitulino wrote: > >>> On Mon, 17 Jun 2013 14:33:10 +0200 > >>> Stefan Hajnoczi wrote: > >>> > >>>> On Mon, Jun 17, 2013 at 11:46:19AM +0200, Pavel Hrdina wrote: > >>>>> On 5.6.2013 15:23, Stefan Hajnoczi wrote: > >>>>>> On Wed, May 29, 2013 at 06:18:19PM +0200, Pavel Hrdina wrote: > >>>>>>> @@ -1071,14 +1072,18 @@ static void qmp_bdrv_open_encrypted(BlockDriverState *bs, const char *filename, > >>>>>>> if (password) { > >>>>>>> if (bdrv_set_key(bs, password) < 0) { > >>>>>>> error_set(errp, QERR_INVALID_PASSWORD); > >>>>>>> + return; > >>>>>>> } > >>>>>>> } else { > >>>>>>> error_set(errp, QERR_DEVICE_ENCRYPTED, bdrv_get_device_name(bs), > >>>>>>> bdrv_get_encrypted_filename(bs)); > >>>>>>> + return; > >>>>>>> } > >>>>>>> } else if (password) { > >>>>>>> error_set(errp, QERR_DEVICE_NOT_ENCRYPTED, bdrv_get_device_name(bs)); > >>>>>>> } > >>>>>>> + > >>>>>>> + bdrv_dev_change_media_cb(bs, true); > >>>>>>> } > >>>>>> > >>>>>> Calling bdrv_dev_change_media_cb() after raising > >>>>>> QERR_DEVICE_NOT_ENCRYPTED is intentional? It might warrant a comment. > >>>>>> > >>>>> > >>>>> Sorry for my late answer. > >>>>> > >>>>> 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 agree, failing for real is probably incompatible by now. I'm tempted to just drop the error then. But it's a very good idea to isolate this change on its own commit with a clear changelog, so that it's easy to identify it and revert case it also generates compatibility problems in the future.