On Mon, 17 Jun 2013 15:38:23 +0200 Pavel Hrdina <phrd...@redhat.com> wrote:
> On 17.6.2013 15:32, Luiz Capitulino wrote: > > On Mon, 17 Jun 2013 15:25:24 +0200 > > Pavel Hrdina <phrd...@redhat.com> wrote: > > > >> On 17.6.2013 15:22, Luiz Capitulino wrote: > >>> On Mon, 17 Jun 2013 14:33:10 +0200 > >>> Stefan Hajnoczi <stefa...@gmail.com> 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.