Skip to contentSkip to conversation

block: fix spurious DEVICE_TRAY_MOVED events on shutdown

PATCH Superseded Pavel Hrdina 4 participants 18 messages May 29, 2013 – Jun 18, 2013

v2 of 4 Superseded Latest is v4 →

lore
Share & tools
lore

Pavel Hrdina

author
#0 lore raw

v2 cover block: fix spurious DEVICE_TRAY_MOVED events on shutdown

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(-)
Signature · 1 line
1.8.1.4
author
#1 lore raw

PATCH v2 1/2 block: make bdrv_dev_change_media_cb() public

From: Luiz Capitulino <[email protected]>

Next commit wants to use it.

Signed-off-by Luiz Capitulino <[email protected]>
Signed-off-by Pavel Hrdina <[email protected]>
---
 block.c               | 3 +--
 include/block/block.h | 1 +
 2 files changed, 2 insertions(+), 2 deletions(-)
block.c +1 -2
index 3f87489..99bc357 100644
@@ -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);
include/block/block.h +1 -0
index 1251c5c..7a238bc 100644
@@ -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,
Signature · 1 line
1.8.1.4
author
#2 lore raw

PATCH v2 2/2 block: move the bdrv_dev_change_media_cb()

In reply to 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 <[email protected]>
---
 block.c    | 8 --------
 blockdev.c | 5 +++++
 2 files changed, 5 insertions(+), 8 deletions(-)
block.c +0 -8
index 99bc357..a64a9ec 100644
@@ -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;
 }
blockdev.c +5 -0
index d1ec99a..a9273de 100644
@@ -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,
Signature · 1 line
1.8.1.4
Tested-by on the v2 series
#17 lore raw
In reply to Pavel Hrdina
Quoted 11 lines · On Wed, 29 May 2013 18:18:17 +0200 Pavel Hrdina <[email protected]> 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 <[email protected]>
Quoted 12 lines

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(-)
#3 lore raw

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

In reply to Pavel Hrdina
Quoted 7 lines · Pavel Hrdina
blockdev.c +1 -0
@@ -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 {

blockdev.c static void qmp_bdrv_open_encrypted(BlockDriverState *bs, const char *filename, · line 1087

Quoted 10 lines
blockdev.c +3 -0
             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.

author
#4 lore raw

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

Quoted 13 lines · Stefan Hajnoczi
Quoted 12 lines · Pavel Hrdina
blockdev.c +2 -0
@@ -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) {
Quoted 9 lines

blockdev.c static void qmp_bdrv_open_encrypted(BlockDriverState *bs, const char *filename, · line 1087

Quoted 5 lines
blockdev.c +2 -0
          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

#5 lore raw

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

Quoted 24 lines · Pavel Hrdina
Quoted 13 lines · Stefan Hajnoczi
Quoted 12 lines · Pavel Hrdina
blockdev.c +2 -0
@@ -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) {
Quoted 9 lines

blockdev.c static void qmp_bdrv_open_encrypted(BlockDriverState *bs, const char *filename, · line 1087

Quoted 5 lines
blockdev.c +2 -0
         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.

Quoted 5 lines

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

#6 lore raw

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

Quoted 25 lines · On Mon, 17 Jun 2013 14:33:10 +0200 Stefan Hajnoczi <[email protected]> wrote:
Quoted 24 lines · Pavel Hrdina
Quoted 22 lines · Stefan Hajnoczi
Quoted 7 lines · Pavel Hrdina
blockdev.c +1 -0
@@ -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 {

blockdev.c static void qmp_bdrv_open_encrypted(BlockDriverState *bs, const char *filename, · line 1087

Quoted 10 lines
blockdev.c +3 -0
             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.

Quoted 8 lines
Quoted 5 lines

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.

author
#7 lore raw

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

Quoted 34 lines · Luiz Capitulino
Quoted 31 lines · On Mon, 17 Jun 2013 14:33:10 +0200 Stefan Hajnoczi <[email protected]> wrote:
Quoted 29 lines · Pavel Hrdina
Quoted 13 lines · Stefan Hajnoczi
Quoted 12 lines · Pavel Hrdina
blockdev.c +2 -0
@@ -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) {
Quoted 9 lines

blockdev.c static void qmp_bdrv_open_encrypted(BlockDriverState *bs, const char *filename, · line 1087

Quoted 5 lines
blockdev.c +2 -0
          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() ).

Quoted 6 lines
Quoted 2 lines

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

#8 lore raw

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

Quoted 38 lines · On Mon, 17 Jun 2013 15:25:24 +0200 Pavel Hrdina <[email protected]> wrote:
Quoted 37 lines · Luiz Capitulino
Quoted 33 lines · On Mon, 17 Jun 2013 14:33:10 +0200 Stefan Hajnoczi <[email protected]> wrote:
Quoted 24 lines · Pavel Hrdina
Quoted 13 lines · Stefan Hajnoczi
Quoted 12 lines · Pavel Hrdina
blockdev.c +2 -0
@@ -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) {
Quoted 9 lines

blockdev.c static void qmp_bdrv_open_encrypted(BlockDriverState *bs, const char *filename, · line 1087

Quoted 5 lines
blockdev.c +2 -0
          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.

Quoted 5 lines

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

Quoted 6 lines
Quoted 3 lines

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.

author
#9 lore raw

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

Quoted 41 lines · Luiz Capitulino
Quoted 38 lines · On Mon, 17 Jun 2013 15:25:24 +0200 Pavel Hrdina <[email protected]> wrote:
Quoted 37 lines · Luiz Capitulino
Quoted 33 lines · On Mon, 17 Jun 2013 14:33:10 +0200 Stefan Hajnoczi <[email protected]> wrote:
Quoted 24 lines · Pavel Hrdina
Quoted 13 lines · Stefan Hajnoczi
Quoted 12 lines · Pavel Hrdina
(quoted diff) +2 -0
@@ -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) {
Quoted 9 lines

Review static void qmp_bdrv_open_encrypted(BlockDriverState *bs, const char *filename, · line 1087

Quoted 5 lines
(quoted diff) +2 -0
           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.

Quoted 5 lines

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

Quoted 9 lines
Quoted 6 lines
Quoted 3 lines

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

#10 lore raw

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

Quoted 49 lines · On Mon, 17 Jun 2013 15:38:23 +0200 Pavel Hrdina <[email protected]> wrote:
Quoted 48 lines · Luiz Capitulino
Quoted 44 lines · On Mon, 17 Jun 2013 15:25:24 +0200 Pavel Hrdina <[email protected]> wrote:
Quoted 34 lines · Luiz Capitulino
Quoted 31 lines · On Mon, 17 Jun 2013 14:33:10 +0200 Stefan Hajnoczi <[email protected]> wrote:
Quoted 29 lines · Pavel Hrdina
Quoted 13 lines · Stefan Hajnoczi
Quoted 12 lines · Pavel Hrdina
(quoted diff) +2 -0
@@ -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) {
Quoted 9 lines

Review static void qmp_bdrv_open_encrypted(BlockDriverState *bs, const char *filename, · line 1087

Quoted 5 lines
(quoted diff) +2 -0
           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() ).

Quoted 6 lines
Quoted 2 lines

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.

Quoted 8 lines
Quoted 2 lines

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.

#11 lore raw

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

In reply to Pavel Hrdina

Am 17.06.2013 um 15:38 hat Pavel Hrdina geschrieben:

Quoted 14 lines
Quoted 14 lines
Quoted 13 lines
Quoted 10 lines
Quoted 6 lines
Quoted 3 lines

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.

Quoted 8 lines
Quoted 2 lines

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

#12 lore raw

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

Quoted 22 lines · On Mon, 17 Jun 2013 15:46:52 +0200 Kevin Wolf <[email protected]> wrote:

Am 17.06.2013 um 15:38 hat Pavel Hrdina geschrieben:

Quoted 21 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 6 lines
Quoted 3 lines

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

Quoted 9 lines
Quoted 6 lines
Quoted 3 lines

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?

Quoted 5 lines
Quoted 1 line

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

#13 lore raw

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

Am 17.06.2013 um 15:51 hat Luiz Capitulino geschrieben:

Quoted 31 lines
Quoted 22 lines · On Mon, 17 Jun 2013 15:46:52 +0200 Kevin Wolf <[email protected]> wrote:

Am 17.06.2013 um 15:38 hat Pavel Hrdina geschrieben:

Quoted 21 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 6 lines
Quoted 3 lines

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

Quoted 9 lines
Quoted 6 lines
Quoted 3 lines

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?

Quoted 5 lines
Quoted 1 line

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.

Quoted 5 lines

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

#15 lore raw

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

In reply to Luiz Capitulino

Am 17.06.2013 um 16:59 hat Luiz Capitulino geschrieben:

Quoted 47 lines
Quoted 36 lines · On Mon, 17 Jun 2013 16:49:11 +0200 Kevin Wolf <[email protected]> wrote:

Am 17.06.2013 um 15:51 hat Luiz Capitulino geschrieben:

Quoted 35 lines
Quoted 22 lines · On Mon, 17 Jun 2013 15:46:52 +0200 Kevin Wolf <[email protected]> wrote:

Am 17.06.2013 um 15:38 hat Pavel Hrdina geschrieben:

Quoted 21 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 6 lines
Quoted 3 lines

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

Quoted 9 lines
Quoted 6 lines
Quoted 3 lines

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?

Quoted 5 lines
Quoted 1 line

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

Quoted 7 lines
Quoted 1 line

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.

Quoted 6 lines

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

#14 lore raw

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

In reply to Kevin Wolf
Quoted 36 lines · On Mon, 17 Jun 2013 16:49:11 +0200 Kevin Wolf <[email protected]> wrote:

Am 17.06.2013 um 15:51 hat Luiz Capitulino geschrieben:

Quoted 35 lines
Quoted 22 lines · On Mon, 17 Jun 2013 15:46:52 +0200 Kevin Wolf <[email protected]> wrote:

Am 17.06.2013 um 15:38 hat Pavel Hrdina geschrieben:

Quoted 21 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 6 lines
Quoted 3 lines

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

Quoted 9 lines
Quoted 6 lines
Quoted 3 lines

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?

Quoted 5 lines
Quoted 1 line

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

Quoted 7 lines
Quoted 1 line

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.

author
#16 lore raw

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

In reply to Kevin Wolf
Quoted 52 lines · Kevin Wolf

Am 17.06.2013 um 16:59 hat Luiz Capitulino geschrieben:

Quoted 51 lines
Quoted 36 lines · On Mon, 17 Jun 2013 16:49:11 +0200 Kevin Wolf <[email protected]> wrote:

Am 17.06.2013 um 15:51 hat Luiz Capitulino geschrieben:

Quoted 35 lines
Quoted 22 lines · On Mon, 17 Jun 2013 15:46:52 +0200 Kevin Wolf <[email protected]> wrote:

Am 17.06.2013 um 15:38 hat Pavel Hrdina geschrieben:

Quoted 21 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 6 lines
Quoted 3 lines

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

Quoted 9 lines
Quoted 6 lines
Quoted 3 lines

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?

Quoted 5 lines
Quoted 1 line

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

Quoted 7 lines
Quoted 1 line

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.

Quoted 7 lines
Quoted 2 lines

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