[libvirt] [PATCH] qemu: add capability probing for block-stream
Since block-stream is not supported on qemu-kvm, so libvirt should post more accurate error info when do blockpull with qemu-kvm but not "Command 'block-stream' is not found" Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1140981 Signed-off-by: Shanzhi Yu <shyu@redhat.com> --- src/qemu/qemu_capabilities.c | 2 ++ src/qemu/qemu_capabilities.h | 1 + src/qemu/qemu_driver.c | 13 +++++-------- 3 files changed, 8 insertions(+), 8 deletions(-) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 81ada48..5674f4f 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -269,6 +269,7 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST, "splash-timeout", /* 175 */ "iothread", + "block-stream", ); @@ -1426,6 +1427,7 @@ struct virQEMUCapsStringFlags virQEMUCapsCommands[] = { { "query-spice", QEMU_CAPS_SPICE }, { "query-kvm", QEMU_CAPS_KVM }, { "block-commit", QEMU_CAPS_BLOCK_COMMIT }, + { "block-stream", QEMU_CAPS_BLOCK_STREAM }, { "query-vnc", QEMU_CAPS_VNC }, { "drive-mirror", QEMU_CAPS_DRIVE_MIRROR }, { "blockdev-snapshot-sync", QEMU_CAPS_DISK_SNAPSHOT }, diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index 0980c00..6701965 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -216,6 +216,7 @@ typedef enum { QEMU_CAPS_RTC_RESET_REINJECTION = 174, /* rtc-reset-reinjection monitor command */ QEMU_CAPS_SPLASH_TIMEOUT = 175, /* -boot splash-time */ QEMU_CAPS_OBJECT_IOTHREAD = 176, /* -object iothread */ + QEMU_CAPS_BLOCK_STREAM = 177, /* block-stream */ QEMU_CAPS_LAST, /* this must always be the last item */ } virQEMUCapsFlags; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 73edda3..b38bc91 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -14980,15 +14980,12 @@ qemuDomainBlockJobImpl(virDomainObjPtr vm, virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("block jobs not supported with this QEMU binary")); goto cleanup; - } else if (base) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("partial block pull not supported with this " - "QEMU binary")); - goto cleanup; - } else if (mode == BLOCK_JOB_PULL && bandwidth) { + } + + if (mode == BLOCK_JOB_PULL && + !(virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BLOCK_STREAM))){ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("setting bandwidth at start of block pull not " - "supported with this QEMU binary")); + _("block pull is not supported with this QEMU binary")); goto cleanup; } -- 1.8.3.1
Ping? On 09/16/2014 01:06 PM, Shanzhi Yu wrote:
Since block-stream is not supported on qemu-kvm, so libvirt should post more accurate error info when do blockpull with qemu-kvm but not "Command 'block-stream' is not found"
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1140981 Signed-off-by: Shanzhi Yu <shyu@redhat.com> --- src/qemu/qemu_capabilities.c | 2 ++ src/qemu/qemu_capabilities.h | 1 + src/qemu/qemu_driver.c | 13 +++++-------- 3 files changed, 8 insertions(+), 8 deletions(-)
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 81ada48..5674f4f 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -269,6 +269,7 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST,
"splash-timeout", /* 175 */ "iothread", + "block-stream", );
@@ -1426,6 +1427,7 @@ struct virQEMUCapsStringFlags virQEMUCapsCommands[] = { { "query-spice", QEMU_CAPS_SPICE }, { "query-kvm", QEMU_CAPS_KVM }, { "block-commit", QEMU_CAPS_BLOCK_COMMIT }, + { "block-stream", QEMU_CAPS_BLOCK_STREAM }, { "query-vnc", QEMU_CAPS_VNC }, { "drive-mirror", QEMU_CAPS_DRIVE_MIRROR }, { "blockdev-snapshot-sync", QEMU_CAPS_DISK_SNAPSHOT }, diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index 0980c00..6701965 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -216,6 +216,7 @@ typedef enum { QEMU_CAPS_RTC_RESET_REINJECTION = 174, /* rtc-reset-reinjection monitor command */ QEMU_CAPS_SPLASH_TIMEOUT = 175, /* -boot splash-time */ QEMU_CAPS_OBJECT_IOTHREAD = 176, /* -object iothread */ + QEMU_CAPS_BLOCK_STREAM = 177, /* block-stream */
QEMU_CAPS_LAST, /* this must always be the last item */ } virQEMUCapsFlags; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 73edda3..b38bc91 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -14980,15 +14980,12 @@ qemuDomainBlockJobImpl(virDomainObjPtr vm, virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("block jobs not supported with this QEMU binary")); goto cleanup; - } else if (base) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("partial block pull not supported with this " - "QEMU binary")); - goto cleanup; - } else if (mode == BLOCK_JOB_PULL && bandwidth) { + } + + if (mode == BLOCK_JOB_PULL && + !(virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BLOCK_STREAM))){ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("setting bandwidth at start of block pull not " - "supported with this QEMU binary")); + _("block pull is not supported with this QEMU binary")); goto cleanup; }
-- Regards shyu
On 09/15/2014 11:06 PM, Shanzhi Yu wrote:
Since block-stream is not supported on qemu-kvm, so libvirt should post more accurate error info when do blockpull with qemu-kvm but not "Command 'block-stream' is not found"
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1140981 Signed-off-by: Shanzhi Yu <shyu@redhat.com> --- src/qemu/qemu_capabilities.c | 2 ++ src/qemu/qemu_capabilities.h | 1 + src/qemu/qemu_driver.c | 13 +++++-------- 3 files changed, 8 insertions(+), 8 deletions(-)
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 81ada48..5674f4f 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -269,6 +269,7 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST,
"splash-timeout", /* 175 */ "iothread", + "block-stream", );
@@ -1426,6 +1427,7 @@ struct virQEMUCapsStringFlags virQEMUCapsCommands[] = { { "query-spice", QEMU_CAPS_SPICE }, { "query-kvm", QEMU_CAPS_KVM }, { "block-commit", QEMU_CAPS_BLOCK_COMMIT }, + { "block-stream", QEMU_CAPS_BLOCK_STREAM },
This part seems okay (we really haven't tracked this before?)...
+++ b/src/qemu/qemu_driver.c @@ -14980,15 +14980,12 @@ qemuDomainBlockJobImpl(virDomainObjPtr vm, virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("block jobs not supported with this QEMU binary")); goto cleanup; - } else if (base) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("partial block pull not supported with this " - "QEMU binary")); - goto cleanup; - } else if (mode == BLOCK_JOB_PULL && bandwidth) { + } + + if (mode == BLOCK_JOB_PULL && + !(virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BLOCK_STREAM))){
But here you are throwing away existing useful error messages for other situations (such as when block-stream exists, but is too old to support a base). I think the flow would look a bit better as: if (..._ASYNC) { async = true; } else if (!..._SYNC) { error: block jobs unsupported + } else if (mode == BLOCK_JOB_PULL && !...STREAM) { + error: block pull unsupported } else if (base) { error: partial pull unsupported } else if (mode == BLOCK_JOB_PULL && bandwidth) { error: bandwidth unsupported } -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 10/24/2014 01:25 PM, Eric Blake wrote:
On 09/15/2014 11:06 PM, Shanzhi Yu wrote:
Since block-stream is not supported on qemu-kvm, so libvirt should post more accurate error info when do blockpull with qemu-kvm but not "Command 'block-stream' is not found"
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1140981 Signed-off-by: Shanzhi Yu <shyu@redhat.com> --- src/qemu/qemu_capabilities.c | 2 ++ src/qemu/qemu_capabilities.h | 1 + src/qemu/qemu_driver.c | 13 +++++-------- 3 files changed, 8 insertions(+), 8 deletions(-)
Hmm, I did a bit more digging. When I originally wrote the code, I used the presence of 'block-job-cancel' (new style, BLOCKJOB_ASYNC) vs. 'block_job_cancel' (old style, BLOCKJOB_SYNC) as a witness whether either style of block jobs was supported. However, it appears that RHEL qemu-kvm is providing block-job-cancel EVEN THOUGH it lacks all methods for starting a block job, which is different from upstream (in upstream, even with the older spelling block_job_cancel, cancel was only introduced at the same time as block-stream). So this is more of a downstream issue - if RHEL qemu-kvm is going to cripple block jobs, they should do so by also crippling cancellation of jobs.
But here you are throwing away existing useful error messages for other situations (such as when block-stream exists, but is too old to support a base). I think the flow would look a bit better as:
if (..._ASYNC) { async = true; } else if (!..._SYNC) { error: block jobs unsupported + } else if (mode == BLOCK_JOB_PULL && !...STREAM) { + error: block pull unsupported } else if (base) { error: partial pull unsupported } else if (mode == BLOCK_JOB_PULL && bandwidth) { error: bandwidth unsupported }
Or even simpler - change how we compute whether BLOCKJOB_SYNC capability is probed, to refuse to set it if block-stream is missing. By changing _just_ qemu_capabilities.c, the existing code here in qemu_driver.c would automatically start printing a nice error message when targetting crippled RHEL qemu. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
participants (2)
-
Eric Blake -
Shanzhi Yu