[Qemu-devel] [PATCH 1/4] migrate: add migration blockers

[Qemu-devel] [PATCH 1/4] migrate: add migration blockers

[Anthony Liguori]

This lets different subsystems register an Error that is thrown whenever
migration is attempted.  This works nicely because it gracefully supports
things like hotplug.

Right now, if multiple errors are registered, only one of them is reported.
I expect that for 1.1, we'll extend query-migrate to return all of the reasons
why migration is disabled at any given point in time.

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
 migration.c |   18 ++++++++++++++++++
 migration.h |   15 +++++++++++++++
 2 files changed, 33 insertions(+), 0 deletions(-)

diff --git a/migration.c b/migration.c
index 41c3c24..6764d3a 100644
--- a/migration.c
+++ b/migration.c
@@ -398,6 +398,18 @@ static MigrationState *migrate_init(Monitor *mon, int detach, int blk, int inc)
     return s;
 }
 
+static GSList *migration_blockers;
+
+void migrate_add_blocker(Error *reason)
+{
+    migration_blockers = g_slist_prepend(migration_blockers, reason);
+}
+
+void migrate_del_blocker(Error *reason)
+{
+    migration_blockers = g_slist_remove(migration_blockers, reason);
+}
+
 int do_migrate(Monitor *mon, const QDict *qdict, QObject **ret_data)
 {
     MigrationState *s = migrate_get_current();
@@ -417,6 +429,12 @@ int do_migrate(Monitor *mon, const QDict *qdict, QObject **ret_data)
         return -1;
     }
 
+    if (migration_blockers) {
+        Error *err = migration_blockers->data;
+        qerror_report_err(err);
+        return -1;
+    }
+
     s = migrate_init(mon, detach, blk, inc);
 
     if (strstart(uri, "tcp:", &p)) {
diff --git a/migration.h b/migration.h
index 1b8ee58..0682179 100644
--- a/migration.h
+++ b/migration.h
@@ -17,6 +17,7 @@
 #include "qdict.h"
 #include "qemu-common.h"
 #include "notify.h"
+#include "error.h"
 
 typedef struct MigrationState MigrationState;
 
@@ -89,4 +90,18 @@ int ram_load(QEMUFile *f, void *opaque, int version_id);
 
 extern int incoming_expected;
 
+/**
+ * @migrate_add_blocker - prevent migration from proceeding
+ *
+ * @reason - an error to be returned whenever migration is attempted
+ */
+void migrate_add_blocker(Error *reason);
+
+/**
+ * @migrate_del_blocker - remove a blocking error from migration
+ *
+ * @reason - the error blocking migration
+ */
+void migrate_del_blocker(Error *reason);
+
 #endif
-- 
1.7.4.1

[Qemu-devel] [PATCH 2/4] ivshmem: use migration blockers to prevent live migration in peer mode

[Anthony Liguori]

Now when you try to migrate with ivshmem, you get a proper QMP error:

(qemu) migrate tcp:localhost:1025
Migration is disabled when using feature 'peer mode' in device 'ivshmem'
(qemu)

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
 hw/ivshmem.c |   12 +++++++++++-
 qerror.c     |    4 ++++
 qerror.h     |    3 +++
 3 files changed, 18 insertions(+), 1 deletions(-)

diff --git a/hw/ivshmem.c b/hw/ivshmem.c
index 242fbea..a3a0e98 100644
--- a/hw/ivshmem.c
+++ b/hw/ivshmem.c
@@ -18,6 +18,8 @@
 #include "pci.h"
 #include "msix.h"
 #include "kvm.h"
+#include "migration.h"
+#include "qerror.h"
 
 #include <sys/mman.h>
 #include <sys/types.h>
@@ -78,6 +80,8 @@ typedef struct IVShmemState {
     uint32_t features;
     EventfdEntry *eventfd_table;
 
+    Error *migration_blocker;
+
     char * shmobj;
     char * sizearg;
     char * role;
@@ -646,7 +650,8 @@ static int pci_ivshmem_init(PCIDevice *dev)
     }
 
     if (s->role_val == IVSHMEM_PEER) {
-        register_device_unmigratable(&s->dev.qdev, "ivshmem", s);
+        error_set(&s->migration_blocker, QERR_DEVICE_FEATURE_BLOCKS_MIGRATION, "ivshmem", "peer mode");
+        migrate_add_blocker(s->migration_blocker);
     }
 
     pci_conf = s->dev.config;
@@ -741,6 +746,11 @@ static int pci_ivshmem_uninit(PCIDevice *dev)
 {
     IVShmemState *s = DO_UPCAST(IVShmemState, dev, dev);
 
+    if (s->migration_blocker) {
+        migrate_del_blocker(s->migration_blocker);
+        error_free(s->migration_blocker);
+    }
+
     memory_region_destroy(&s->ivshmem_mmio);
     memory_region_del_subregion(&s->bar, &s->ivshmem);
     memory_region_destroy(&s->ivshmem);
diff --git a/qerror.c b/qerror.c
index 4b48b39..8e30e2d 100644
--- a/qerror.c
+++ b/qerror.c
@@ -73,6 +73,10 @@ static const QErrorStringTable qerror_table[] = {
         .desc      = "Device '%(device)' is in use",
     },
     {
+        .error_fmt = QERR_DEVICE_FEATURE_BLOCKS_MIGRATION,
+        .desc      = "Migration is disabled when using feature '%(feature)' in device '%(device)'",
+    },
+    {
         .error_fmt = QERR_DEVICE_LOCKED,
         .desc      = "Device '%(device)' is locked",
     },
diff --git a/qerror.h b/qerror.h
index d4bfcfd..7e2eebf 100644
--- a/qerror.h
+++ b/qerror.h
@@ -72,6 +72,9 @@ QError *qobject_to_qerror(const QObject *obj);
 #define QERR_DEVICE_IN_USE \
     "{ 'class': 'DeviceInUse', 'data': { 'device': %s } }"
 
+#define QERR_DEVICE_FEATURE_BLOCKS_MIGRATION \
+    "{ 'class': 'DeviceFeatureBlocksMigration', 'data': { 'device': %s, 'feature': %s } }"
+
 #define QERR_DEVICE_LOCKED \
     "{ 'class': 'DeviceLocked', 'data': { 'device': %s } }"
 
-- 
1.7.4.1

[Qemu-devel] [PATCH 3/4] qcow2: add a migration blocker

[Anthony Liguori]

Now when you try to migrate with qcow2, you get:

(qemu) migrate tcp:localhost:1025
Block format 'qcow2' used by device 'ide0-hd0' does not support feature 'live migration'
(qemu)

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
 block/qcow2.c |    9 +++++++++
 block/qcow2.h |    2 ++
 qemu-tool.c   |    9 +++++++++
 qerror.c      |    4 ++++
 qerror.h      |    3 +++
 5 files changed, 27 insertions(+), 0 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index ef057d3..500cbbf 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -29,6 +29,7 @@
 #include "block/qcow2.h"
 #include "qemu-error.h"
 #include "qerror.h"
+#include "migration.h"
 
 /*
   Differences with QCOW:
@@ -277,6 +278,11 @@ static int qcow2_open(BlockDriverState *bs, int flags)
         goto fail;
     }
 
+    error_set(&s->migration_blocker,
+              QERR_BLOCK_FORMAT_FEATURE_NOT_SUPPORTED,
+              "qcow2", bs->device_name, "live migration");
+    migrate_add_blocker(s->migration_blocker);
+
     /* Initialise locks */
     qemu_co_mutex_init(&s->lock);
 
@@ -621,6 +627,9 @@ static void qcow2_close(BlockDriverState *bs)
     BDRVQcowState *s = bs->opaque;
     g_free(s->l1_table);
 
+    migrate_del_blocker(s->migration_blocker);
+    error_free(s->migration_blocker);
+
     qcow2_cache_flush(bs, s->l2_table_cache);
     qcow2_cache_flush(bs, s->refcount_block_cache);
 
diff --git a/block/qcow2.h b/block/qcow2.h
index 531af39..0734b23 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -125,6 +125,8 @@ typedef struct BDRVQcowState {
     int snapshots_size;
     int nb_snapshots;
     QCowSnapshot *snapshots;
+
+    Error *migration_blocker;
 } BDRVQcowState;
 
 /* XXX: use std qcow open function ? */
diff --git a/qemu-tool.c b/qemu-tool.c
index e9f7fe1..5df7279 100644
--- a/qemu-tool.c
+++ b/qemu-tool.c
@@ -15,6 +15,7 @@
 #include "monitor.h"
 #include "qemu-timer.h"
 #include "qemu-log.h"
+#include "migration.h"
 
 #include <sys/time.h>
 
@@ -92,3 +93,11 @@ int64_t qemu_get_clock_ns(QEMUClock *clock)
 {
     return 0;
 }
+
+void migrate_add_blocker(Error *reason)
+{
+}
+
+void migrate_del_blocker(Error *reason)
+{
+}
diff --git a/qerror.c b/qerror.c
index 8e30e2d..fdf62b9 100644
--- a/qerror.c
+++ b/qerror.c
@@ -49,6 +49,10 @@ static const QErrorStringTable qerror_table[] = {
         .desc      = "Device '%(device)' can't go on a %(bad_bus_type) bus",
     },
     {
+        .error_fmt = QERR_BLOCK_FORMAT_FEATURE_NOT_SUPPORTED,
+        .desc      = "Block format '%(format)' used by device '%(name)' does not support feature '%(feature)'",
+    },
+    {
         .error_fmt = QERR_BUS_NOT_FOUND,
         .desc      = "Bus '%(bus)' not found",
     },
diff --git a/qerror.h b/qerror.h
index 7e2eebf..2d3d43b 100644
--- a/qerror.h
+++ b/qerror.h
@@ -54,6 +54,9 @@ QError *qobject_to_qerror(const QObject *obj);
 #define QERR_BAD_BUS_FOR_DEVICE \
     "{ 'class': 'BadBusForDevice', 'data': { 'device': %s, 'bad_bus_type': %s } }"
 
+#define QERR_BLOCK_FORMAT_FEATURE_NOT_SUPPORTED \
+    "{ 'class': 'BlockFormatFeatureNotSupported', 'data': { 'format': %s, 'name': %s, 'feature': %s } }"
+
 #define QERR_BUS_NOT_FOUND \
     "{ 'class': 'BusNotFound', 'data': { 'bus': %s } }"
 
-- 
1.7.4.1

[Qemu-devel] [PATCH 4/4] qed: add migration blocker

[Anthony Liguori]

Now when you try to migrate with qed, you get:

(qemu) migrate tcp:localhost:1025
Block format 'qed' used by device 'ide0-hd0' does not support feature 'live migration'
(qemu)

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
 block/qed.c |   10 ++++++++++
 block/qed.h |    2 ++
 2 files changed, 12 insertions(+), 0 deletions(-)

diff --git a/block/qed.c b/block/qed.c
index d032a45..7e22e77 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -16,6 +16,7 @@
 #include "trace.h"
 #include "qed.h"
 #include "qerror.h"
+#include "migration.h"
 
 static void qed_aio_cancel(BlockDriverAIOCB *blockacb)
 {
@@ -504,6 +505,12 @@ static int bdrv_qed_open(BlockDriverState *bs, int flags)
     s->need_check_timer = qemu_new_timer_ns(vm_clock,
                                             qed_need_check_timer_cb, s);
 
+    error_set(&s->migration_blocker,
+              QERR_BLOCK_FORMAT_FEATURE_NOT_SUPPORTED,
+              "qed", bs->device_name, "live migration");
+    migrate_add_blocker(s->migration_blocker);
+
+
 out:
     if (ret) {
         qed_free_l2_cache(&s->l2_cache);
@@ -516,6 +523,9 @@ static void bdrv_qed_close(BlockDriverState *bs)
 {
     BDRVQEDState *s = bs->opaque;
 
+    migrate_del_blocker(s->migration_blocker);
+    error_free(s->migration_blocker);
+
     qed_cancel_need_check_timer(s);
     qemu_free_timer(s->need_check_timer);
 
diff --git a/block/qed.h b/block/qed.h
index 388fdb3..62cbd3b 100644
--- a/block/qed.h
+++ b/block/qed.h
@@ -164,6 +164,8 @@ typedef struct {
 
     /* Periodic flush and clear need check flag */
     QEMUTimer *need_check_timer;
+
+    Error *migration_blocker;
 } BDRVQEDState;
 
 enum {
-- 
1.7.4.1

Re: [Qemu-devel] [PATCH 2/4] ivshmem: use migration blockers to prevent live migration in peer mode

[Aneesh Kumar K.V]

On Fri, 11 Nov 2011 14:29:36 -0600, Anthony Liguori <aliguori@us.ibm.com> wrote:
> Now when you try to migrate with ivshmem, you get a proper QMP error:
> 
> (qemu) migrate tcp:localhost:1025
> Migration is disabled when using feature 'peer mode' in device 'ivshmem'
> (qemu)
> 

We may want to add a similar one when using VirtFS export. But with
VirtFS there is no easy way to do migrate_del_blocker, unless we add a
detach 9p op to map umount.

-aneesh

Re: [Qemu-devel] [PATCH 3/4] qcow2: add a migration blocker

[Stefan Hajnoczi]

On Fri, Nov 11, 2011 at 8:29 PM, Anthony Liguori <aliguori@us.ibm.com> wrote:
> +#define QERR_BLOCK_FORMAT_FEATURE_NOT_SUPPORTED \
> +    "{ 'class': 'BlockFormatFeatureNotSupported', 'data': { 'format': %s, 'name': %s, 'feature': %s } }"
> +

Isn't having a separate error going to make life harder for management
tool writers?  I would have expected one "migration not supported"
error, regardless of whether the reason is ivshmem, qcow2, or anything
else.

Stefan

Re: [Qemu-devel] [PATCH 3/4] qcow2: add a migration blocker

[Anthony Liguori]

On 11/14/2011 09:51 AM, Stefan Hajnoczi wrote:
> On Fri, Nov 11, 2011 at 8:29 PM, Anthony Liguori<aliguori@us.ibm.com>  wrote:
>> +#define QERR_BLOCK_FORMAT_FEATURE_NOT_SUPPORTED \
>> +    "{ 'class': 'BlockFormatFeatureNotSupported', 'data': { 'format': %s, 'name': %s, 'feature': %s } }"
>> +
>
> Isn't having a separate error going to make life harder for management
> tool writers?  I would have expected one "migration not supported"
> error, regardless of whether the reason is ivshmem, qcow2, or anything
> else.

Errors shouldn't be tied to verbs.  IOW, if you have a migrate command, you 
don't want to have a MigrationFailed error because that's tied to a specific verb.

Instead, you want the errors to provide additional information about the verb 
failed.  In this case, the verb is failing because you're requesting to use a 
feature that is not supported by this particular block format.

Regards,

Anthony Liguori

>
> Stefan
>

Re: [Qemu-devel] [PATCH 3/4] qcow2: add a migration blocker

[Kevin Wolf]

Am 14.11.2011 17:25, schrieb Anthony Liguori:
> On 11/14/2011 09:51 AM, Stefan Hajnoczi wrote:
>> On Fri, Nov 11, 2011 at 8:29 PM, Anthony Liguori<aliguori@us.ibm.com>  wrote:
>>> +#define QERR_BLOCK_FORMAT_FEATURE_NOT_SUPPORTED \
>>> +    "{ 'class': 'BlockFormatFeatureNotSupported', 'data': { 'format': %s, 'name': %s, 'feature': %s } }"
>>> +
>>
>> Isn't having a separate error going to make life harder for management
>> tool writers?  I would have expected one "migration not supported"
>> error, regardless of whether the reason is ivshmem, qcow2, or anything
>> else.
> 
> Errors shouldn't be tied to verbs.  IOW, if you have a migrate command, you 
> don't want to have a MigrationFailed error because that's tied to a specific verb.
> 
> Instead, you want the errors to provide additional information about the verb 
> failed.  In this case, the verb is failing because you're requesting to use a 
> feature that is not supported by this particular block format.

We already have QERR_UNKNOWN_BLOCK_FORMAT_FEATURE which is very similar
(in fact, FEATURE_NOT_SUPPORTED is a much better name which we should
have used from the beginning).

"unknown feature" doesn't really fit here and it's too late to change
the name of the existing error. We'll probably have to live with two
different error messages.

Kevin

Re: [Qemu-devel] [PATCH 3/4] qcow2: add a migration blocker

[Stefan Hajnoczi]

On Mon, Nov 14, 2011 at 4:25 PM, Anthony Liguori <anthony@codemonkey.ws> wrote:
> On 11/14/2011 09:51 AM, Stefan Hajnoczi wrote:
>>
>> On Fri, Nov 11, 2011 at 8:29 PM, Anthony Liguori<aliguori@us.ibm.com>
>>  wrote:
>>>
>>> +#define QERR_BLOCK_FORMAT_FEATURE_NOT_SUPPORTED \
>>> +    "{ 'class': 'BlockFormatFeatureNotSupported', 'data': { 'format':
>>> %s, 'name': %s, 'feature': %s } }"
>>> +
>>
>> Isn't having a separate error going to make life harder for management
>> tool writers?  I would have expected one "migration not supported"
>> error, regardless of whether the reason is ivshmem, qcow2, or anything
>> else.
>
> Errors shouldn't be tied to verbs.  IOW, if you have a migrate command, you
> don't want to have a MigrationFailed error because that's tied to a specific
> verb.
>
> Instead, you want the errors to provide additional information about the
> verb failed.  In this case, the verb is failing because you're requesting to
> use a feature that is not supported by this particular block format.

Okay, this error is semantically different from the earlier error in
this series.

We need QMP documentation updates in this series so management tool
writers know to look out for.  Otherwise libvirt and friends can only
provide generic "This operation failed.  Opaque QEMU error: <blob>"
rather than responding with graceful error handling.

Stefan

Re: [Qemu-devel] [PATCH 3/4] qcow2: add a migration blocker

[Anthony Liguori]

On 11/14/2011 10:45 AM, Stefan Hajnoczi wrote:
> On Mon, Nov 14, 2011 at 4:25 PM, Anthony Liguori<anthony@codemonkey.ws>  wrote:
>> On 11/14/2011 09:51 AM, Stefan Hajnoczi wrote:
>>>
>>> On Fri, Nov 11, 2011 at 8:29 PM, Anthony Liguori<aliguori@us.ibm.com>
>>>   wrote:
>>>>
>>>> +#define QERR_BLOCK_FORMAT_FEATURE_NOT_SUPPORTED \
>>>> +    "{ 'class': 'BlockFormatFeatureNotSupported', 'data': { 'format':
>>>> %s, 'name': %s, 'feature': %s } }"
>>>> +
>>>
>>> Isn't having a separate error going to make life harder for management
>>> tool writers?  I would have expected one "migration not supported"
>>> error, regardless of whether the reason is ivshmem, qcow2, or anything
>>> else.
>>
>> Errors shouldn't be tied to verbs.  IOW, if you have a migrate command, you
>> don't want to have a MigrationFailed error because that's tied to a specific
>> verb.
>>
>> Instead, you want the errors to provide additional information about the
>> verb failed.  In this case, the verb is failing because you're requesting to
>> use a feature that is not supported by this particular block format.
>
> Okay, this error is semantically different from the earlier error in
> this series.
>
> We need QMP documentation updates in this series so management tool
> writers know to look out for.  Otherwise libvirt and friends can only
> provide generic "This operation failed.  Opaque QEMU error:<blob>"
> rather than responding with graceful error handling.

Current QMP documentation does not document errors.

The QAPI changes document the errors but it also changes the error paths such 
that you can do this in a sane fashion.

So I agree with you 100% but it's a bigger thing than just this series.

Regards,

Anthony Liguori

>
> Stefan
>

Re: [Qemu-devel] [PATCH 3/4] qcow2: add a migration blocker

[Stefan Hajnoczi]

On Mon, Nov 14, 2011 at 4:49 PM, Anthony Liguori <anthony@codemonkey.ws> wrote:
> On 11/14/2011 10:45 AM, Stefan Hajnoczi wrote:
>>
>> On Mon, Nov 14, 2011 at 4:25 PM, Anthony Liguori<anthony@codemonkey.ws>
>>  wrote:
>>>
>>> On 11/14/2011 09:51 AM, Stefan Hajnoczi wrote:
>>>>
>>>> On Fri, Nov 11, 2011 at 8:29 PM, Anthony Liguori<aliguori@us.ibm.com>
>>>>  wrote:
>>>>>
>>>>> +#define QERR_BLOCK_FORMAT_FEATURE_NOT_SUPPORTED \
>>>>> +    "{ 'class': 'BlockFormatFeatureNotSupported', 'data': { 'format':
>>>>> %s, 'name': %s, 'feature': %s } }"
>>>>> +
>>>>
>>>> Isn't having a separate error going to make life harder for management
>>>> tool writers?  I would have expected one "migration not supported"
>>>> error, regardless of whether the reason is ivshmem, qcow2, or anything
>>>> else.
>>>
>>> Errors shouldn't be tied to verbs.  IOW, if you have a migrate command,
>>> you
>>> don't want to have a MigrationFailed error because that's tied to a
>>> specific
>>> verb.
>>>
>>> Instead, you want the errors to provide additional information about the
>>> verb failed.  In this case, the verb is failing because you're requesting
>>> to
>>> use a feature that is not supported by this particular block format.
>>
>> Okay, this error is semantically different from the earlier error in
>> this series.
>>
>> We need QMP documentation updates in this series so management tool
>> writers know to look out for.  Otherwise libvirt and friends can only
>> provide generic "This operation failed.  Opaque QEMU error:<blob>"
>> rather than responding with graceful error handling.
>
> Current QMP documentation does not document errors.
>
> The QAPI changes document the errors but it also changes the error paths
> such that you can do this in a sane fashion.
>
> So I agree with you 100% but it's a bigger thing than just this series.

Okay, I thought about it because I've been documenting errors in the
generic image streaming series.

Stefan