[Qemu-devel] [PATCH v2 0/2] block: fix spurious DEVICE_TRAY_MOVED events on shutdown

[Qemu-devel] [PATCH v2 0/2] block: fix spurious DEVICE_TRAY_MOVED events on shutdown

[Pavel Hrdina]

This fixes a regression introduced by commit 9ca111544.

The first commit is done by Luiz and I've just use it as it is.

The second commit moves the bdrv_dev_change_media_cb() into eject_device(),
called by QMP and HMP eject command, and into qmp_bdrv_open_encrypted(),
called by QMP and HMP change command. These are the only place where I think
that should call the bdrv_dev_change_media_cb() function.

There is no reason to call this function while we are removing the device
from the guest, for example while closing and deleting all devices on shutdown.

Luiz Capitulino (1):
  block: make bdrv_dev_change_media_cb() public

Pavel Hrdina (1):
  block: move the bdrv_dev_change_media_cb()

 block.c               | 11 +----------
 blockdev.c            |  5 +++++
 include/block/block.h |  1 +
 3 files changed, 7 insertions(+), 10 deletions(-)

-- 
1.8.1.4

[Qemu-devel] [PATCH v2 1/2] block: make bdrv_dev_change_media_cb() public

[Pavel Hrdina]

From: Luiz Capitulino <lcapitulino@redhat.com>

Next commit wants to use it.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
---
 block.c               | 3 +--
 include/block/block.h | 1 +
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/block.c b/block.c
index 3f87489..99bc357 100644
--- a/block.c
+++ b/block.c
@@ -56,7 +56,6 @@ typedef enum {
     BDRV_REQ_ZERO_WRITE   = 0x2,
 } BdrvRequestFlags;
 
-static void bdrv_dev_change_media_cb(BlockDriverState *bs, bool load);
 static BlockDriverAIOCB *bdrv_aio_readv_em(BlockDriverState *bs,
         int64_t sector_num, QEMUIOVector *qiov, int nb_sectors,
         BlockDriverCompletionFunc *cb, void *opaque);
@@ -1681,7 +1680,7 @@ static void bdrv_emit_qmp_eject_event(BlockDriverState *bs, bool ejected)
     qobject_decref(data);
 }
 
-static void bdrv_dev_change_media_cb(BlockDriverState *bs, bool load)
+void bdrv_dev_change_media_cb(BlockDriverState *bs, bool load)
 {
     if (bs->dev_ops && bs->dev_ops->change_media_cb) {
         bool tray_was_closed = !bdrv_dev_is_tray_open(bs);
diff --git a/include/block/block.h b/include/block/block.h
index 1251c5c..7a238bc 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -158,6 +158,7 @@ void bdrv_set_dev_ops(BlockDriverState *bs, const BlockDevOps *ops,
                       void *opaque);
 void bdrv_dev_eject_request(BlockDriverState *bs, bool force);
 bool bdrv_dev_has_removable_media(BlockDriverState *bs);
+void bdrv_dev_change_media_cb(BlockDriverState *bs, bool load);
 bool bdrv_dev_is_tray_open(BlockDriverState *bs);
 bool bdrv_dev_is_medium_locked(BlockDriverState *bs);
 int bdrv_read(BlockDriverState *bs, int64_t sector_num,
-- 
1.8.1.4

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

[Pavel Hrdina]

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(-)

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,
-- 
1.8.1.4

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

[Stefan Hajnoczi]

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.

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

[Pavel Hrdina]

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

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

[Stefan Hajnoczi]

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

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

[Luiz Capitulino]

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.

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

[Pavel Hrdina]

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

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

[Luiz Capitulino]

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.

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

[Pavel Hrdina]

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

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

[Luiz Capitulino]

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.

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

[Kevin Wolf]

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

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

[Luiz Capitulino]

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...

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

[Kevin Wolf]

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

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

[Luiz Capitulino]

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.

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

[Kevin Wolf]

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

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

[Pavel Hrdina]

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

Re: [Qemu-devel] [PATCH v2 0/2] block: fix spurious DEVICE_TRAY_MOVED events on shutdown

[Luiz Capitulino]

On Wed, 29 May 2013 18:18:17 +0200
Pavel Hrdina <phrdina@redhat.com> wrote:

> This fixes a regression introduced by commit 9ca111544.
> 
> The first commit is done by Luiz and I've just use it as it is.
> 
> The second commit moves the bdrv_dev_change_media_cb() into eject_device(),
> called by QMP and HMP eject command, and into qmp_bdrv_open_encrypted(),
> called by QMP and HMP change command. These are the only place where I think
> that should call the bdrv_dev_change_media_cb() function.
> 
> There is no reason to call this function while we are removing the device
> from the guest, for example while closing and deleting all devices on shutdown.

I'll defer review to the block guys, but as this works for me:

Tested-by: Luiz Capitulino <lcapitulino@redhat.com>

> 
> Luiz Capitulino (1):
>   block: make bdrv_dev_change_media_cb() public
> 
> Pavel Hrdina (1):
>   block: move the bdrv_dev_change_media_cb()
> 
>  block.c               | 11 +----------
>  blockdev.c            |  5 +++++
>  include/block/block.h |  1 +
>  3 files changed, 7 insertions(+), 10 deletions(-)
>