Re: [libvirt] [PATCH 1/5] blockjob: add qemu capabilities related to block pull jobs
On 04/11/2012 07:09 PM, Daniel Veillard wrote:
On Wed, Apr 11, 2012 at 05:40:34PM -0600, Eric Blake wrote:
> RHEL 6.2 was released with an early version of block jobs, which only
> worked on the qed file format, where the commands were spelled with
> underscore (contrary to QMP style), and where 'block_job_cancel' was
> synchronous and did not trigger an event.
>
> +++ b/src/qemu/qemu_monitor.h
> @@ -538,8 +538,9 @@ int qemuMonitorBlockJob(qemuMonitorPtr mon,
> const char *back,
> unsigned long bandwidth,
> virDomainBlockJobInfoPtr info,
> - int mode)
> - ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(5);
> + int mode,
> + bool async)
> + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
Hum, gcc wasn't complaining on an "ATTRIBUTE_NONNULL(5)" for something
which wasn't a pointer ?
Context. There are lines omitted between the @@ marker and the changed
lines; param 5 is virDomainBlockJobInfoPtr. In fact, I introduced a bug
in commit 10ec36e2 for adding ATTRIBUTE_NONNULL(5) in the first place,
since we already have existing callers that pass NULL.
> + else if (STREQ(name, "block_job_cancel"))
> + qemuCapsSet(qemuCaps, QEMU_CAPS_BLOCKJOB_SYNC);
> + else if (STREQ(name, "block-job-cancel"))
> + qemuCapsSet(qemuCaps, QEMU_CAPS_BLOCKJOB_ASYNC);
> }
Hum ... seems to me that we always set bits QEMU_CAPS_BLOCKJOB_SYNC
and QEMU_CAPS_BLOCKJOB_ASYNC together, so do you envision cases where
one was set and not the other ? If not why not merge them for the sake
of one less bit to manage ?
On the contrary, you will set at most one of the two, and never both.
That is, there is no qemu image that supports both 'block_job_cancel'
and 'block-job-cancel' simultaneously, so we need the two bits to tell
the two styles apart.
Minor point about the extra bit, to me that's fine, ACK
Thanks for the review. I'll see what you said about the rest of the
series before pushing.
--
Eric Blake eblake(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachments:
- signature.asc (application/pgp-signature — 620 bytes)