Message ID | efebd1409316dc6bf2c1fc10a4b933955a6c38e9.1369841836.git.phrdina@redhat.com |
---|---|
State | New |
Headers | show |
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.
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() ). Pavel
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". Stefan
On Mon, 17 Jun 2013 14:33:10 +0200 Stefan Hajnoczi <stefanha@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.
On 17.6.2013 15:22, Luiz Capitulino wrote: > On Mon, 17 Jun 2013 14:33:10 +0200 > Stefan Hajnoczi <stefanha@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. Pavel
On Mon, 17 Jun 2013 15:25:24 +0200 Pavel Hrdina <phrdina@redhat.com> wrote: > On 17.6.2013 15:22, Luiz Capitulino wrote: > > On Mon, 17 Jun 2013 14:33:10 +0200 > > Stefan Hajnoczi <stefanha@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.
On 17.6.2013 15:32, Luiz Capitulino wrote: > On Mon, 17 Jun 2013 15:25:24 +0200 > Pavel Hrdina <phrdina@redhat.com> wrote: > >> On 17.6.2013 15:22, Luiz Capitulino wrote: >>> On Mon, 17 Jun 2013 14:33:10 +0200 >>> Stefan Hajnoczi <stefanha@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. Pavel
On Mon, 17 Jun 2013 15:38:23 +0200 Pavel Hrdina <phrdina@redhat.com> wrote: > On 17.6.2013 15:32, Luiz Capitulino wrote: > > On Mon, 17 Jun 2013 15:25:24 +0200 > > Pavel Hrdina <phrdina@redhat.com> wrote: > > > >> On 17.6.2013 15:22, Luiz Capitulino wrote: > >>> On Mon, 17 Jun 2013 14:33:10 +0200 > >>> Stefan Hajnoczi <stefanha@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.
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. Kevin
On Mon, 17 Jun 2013 15:46:52 +0200 Kevin Wolf <kwolf@redhat.com> 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...
Am 17.06.2013 um 15:51 hat Luiz Capitulino geschrieben: > On Mon, 17 Jun 2013 15:46:52 +0200 > Kevin Wolf <kwolf@redhat.com> 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. Kevin
On Mon, 17 Jun 2013 16:49:11 +0200 Kevin Wolf <kwolf@redhat.com> wrote: > Am 17.06.2013 um 15:51 hat Luiz Capitulino geschrieben: > > On Mon, 17 Jun 2013 15:46:52 +0200 > > Kevin Wolf <kwolf@redhat.com> 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.
Am 17.06.2013 um 16:59 hat Luiz Capitulino geschrieben: > On Mon, 17 Jun 2013 16:49:11 +0200 > Kevin Wolf <kwolf@redhat.com> wrote: > > > Am 17.06.2013 um 15:51 hat Luiz Capitulino geschrieben: > > > On Mon, 17 Jun 2013 15:46:52 +0200 > > > Kevin Wolf <kwolf@redhat.com> 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
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 <kwolf@redhat.com> wrote: >> >>> Am 17.06.2013 um 15:51 hat Luiz Capitulino geschrieben: >>>> On Mon, 17 Jun 2013 15:46:52 +0200 >>>> Kevin Wolf <kwolf@redhat.com> 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
diff --git a/block.c b/block.c index 99bc357..a64a9ec 100644 --- a/block.c +++ b/block.c @@ -1084,10 +1084,6 @@ int bdrv_open(BlockDriverState *bs, const char *filename, QDict *options, } QDECREF(options); - if (!bdrv_key_required(bs)) { - bdrv_dev_change_media_cb(bs, true); - } - /* throttling disk I/O limits */ if (bs->io_limits_enabled) { bdrv_io_limits_enable(bs); @@ -1389,8 +1385,6 @@ void bdrv_close(BlockDriverState *bs) } } - bdrv_dev_change_media_cb(bs, false); - /*throttling disk I/O limits*/ if (bs->io_limits_enabled) { bdrv_io_limits_disable(bs); @@ -2845,8 +2839,6 @@ int bdrv_set_key(BlockDriverState *bs, const char *key) bs->valid_key = 0; } else if (!bs->valid_key) { bs->valid_key = 1; - /* call the change callback now, we skipped it on open */ - bdrv_dev_change_media_cb(bs, true); } return ret; } diff --git a/blockdev.c b/blockdev.c index d1ec99a..a9273de 100644 --- a/blockdev.c +++ b/blockdev.c @@ -1022,6 +1022,7 @@ static void eject_device(BlockDriverState *bs, int force, Error **errp) } bdrv_close(bs); + bdrv_dev_change_media_cb(bs, false); } void qmp_eject(const char *device, bool has_force, bool force, Error **errp) @@ -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); } void qmp_change_blockdev(const char *device, const char *filename,
The bdrv_dev_change_media_cb() should be called only for eject and change commands. We should call that function only if that command is successful. What this function does is that it calls the change_media_cb() and also emit the QEVENT_DEVICE_TRAY_MOVED event. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- block.c | 8 -------- blockdev.c | 5 +++++ 2 files changed, 5 insertions(+), 8 deletions(-)