[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH] qemu: add capability probing for block-stream

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

> 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

Attachment: signature.asc
Description: OpenPGP digital signature

[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]