From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:57362) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UoZdk-0003C0-1R for qemu-devel@nongnu.org; Mon, 17 Jun 2013 09:38:40 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UoZdb-0000v6-4V for qemu-devel@nongnu.org; Mon, 17 Jun 2013 09:38:35 -0400 Received: from mx1.redhat.com ([209.132.183.28]:37535) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UoZda-0000v2-Rw for qemu-devel@nongnu.org; Mon, 17 Jun 2013 09:38:27 -0400 Message-ID: <51BF114F.3000507@redhat.com> Date: Mon, 17 Jun 2013 15:38:23 +0200 From: Pavel Hrdina MIME-Version: 1.0 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> In-Reply-To: <20130617093202.2bd30f17@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: Luiz Capitulino Cc: kwolf@redhat.com, Stefan Hajnoczi , qemu-devel@nongnu.org, armbru@redhat.com 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. Pavel