qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Pavel Hrdina
Subject: Re: [Qemu-devel] [PATCH v2 2/2] block: move the bdrv_dev_change_media_cb()
Date: Tue, 18 Jun 2013 08:26:19 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130514 Thunderbird/17.0.6

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 <address@hidden> wrote:

Am 17.06.2013 um 15:51 hat Luiz Capitulino geschrieben:
On Mon, 17 Jun 2013 15:46:52 +0200
Kevin Wolf <address@hidden> 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



reply via email to

[Prev in Thread] Current Thread [Next in Thread]