| Message ID | 1321043378-15344-3-git-send-email-aliguori@us.ibm.com |
|---|---|
| State | New |
| Headers | show |
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
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 >
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
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
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 >
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
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 } }"
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(-)