Re: [Qemu-devel] [PATCH v2 2/2] block: move the bdrv_dev_change_media_cb()

[Pavel Hrdina <phrdina@redhat.com>]

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