Re: [libvirt] [PATCHv5 08/23] blockjob: add qemu capabilities related to block jobs
On 04/18/2012 02:18 PM, Laine Stump wrote:
On 04/17/2012 01:05 AM, Eric Blake wrote:
> The new block copy storage migration sequence requires both the
> 'drive-mirror' and 'drive-reopen' monitor commands, which have
> been proposed[1] for qemu 1.1. Someday (probably for qemu 1.2),
> these commands may also be added to the 'transaction' monitor
> command for even more power, but we don't need to worry about
> that now.
>
> [1]https://lists.gnu.org/archive/html/qemu-devel/2012-04/msg01630.html
>
> +++ b/src/qemu/qemu_monitor.c
> @@ -2685,6 +2685,31 @@ qemuMonitorDiskSnapshot(qemuMonitorPtr mon, virJSONValuePtr
actions,
> return ret;
> }
>
> +/* Add the drive-mirror action to a transaction. */
Is this comment now incorrect? (You say the new qemu proposal doesn't
allow interaction with "transaction")
Indeed. I'll fix that.
> +int
> +qemuMonitorDriveMirror(qemuMonitorPtr mon,
> + const char *device, const char *file,
> + const char *format, unsigned int flags)
> +{
> + int ret = -1;
> +
> + VIR_DEBUG("mon=%p, device=%s, file=%s, format=%s, flags=%x",
> + mon, device, file, format, flags);
Should we be concerned about NULL string pointers for args that aren't
qualified as ATTRIBUTE_NONNULL?
Which is just format, but yes, I need NULLSTR(format). glibc doesn't
care, but other platforms might.
> +
> + if (!mon) {
I had thought that declaring an arg ATTRIBUTE_NONNULL meant that you
didn't need to check for NULL in the code, so when I saw this, I was
confused and decided to investigate. I found the following gcc bug:
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=17308
conveniently with a comment posted by on "Eric Blake" (any relation? :-)
A blast from my past :)
So does this mean that all of our ATTRIBUTE_NONNULL declarations are
pointless?
Not entirely pointless, since Coverity, clang, and friends are smarter
than gcc, and will actually use ATTRIBUTE_NONNULL to point out bugs that
gcc currently misses. But yes, that gcc bug is quite annoying (and one
of the sad reasons that llvm is gaining popularity, even though it's
license is not as Free [as in freedom] as gcc's). And it gives us all
the more reason to keep running Coverity reports.
I'll avoid using the "A word" because I don't know the status of qemu
upstream, and don't want to confuse patchchecker. It all looks okay to
me though, although the ATTRIBUTE_NONNULL stuff was a bit of a
revelation (probably I'm the only one who didn't already know about it...).
On IRC today, Paolo told me that 'drive-mirror' is looking robust enough
to make it into qemu 1.1, but 'drive-reopen' is floundering and might be
delayed. If that ends up being the case, it would mean that libvirt
could _still_ provide copy semantics
(virDomainBlockRebase(,VIR_DOMAIN_BLOCK_REBASE_COPY) followed by
virDomainBlockJobAbort(,0) after reaching the mirroring phase), but
would lose out on the ability to do migration
(virDomainBlockRebase(,VIR_DOMAIN_BLOCK_REBASE_COPY) followed by
virDomainBlockJobAbort(,VIR_DOMAIN_BLOCK_JOB_ABORT_PIVOT)).
As such, it may mean that I should consider splitting this patch into
two pieces, to be applied independently according to when things are
applied in upstream qemu (and to backport both to RHEL, where both
commands exist as a __com.redhat_ variant).
I _still_ think that patches 4-6 are in solid shape - we know what API
changes to support, once we can map them to a qemu implementation (and
better yet, we can map them to other hypervisors), bringing us back to
my claim that 7 and beyond are still waiting on qemu stability. It's
not the nicest to commit to an API without an implementation, but we've
done it before, and I'm quite confident that no matter what qemu finally
settles on, at least our API decision is robust enough to support that
qemu implementation.
Paolo, any corrections?
--
Eric Blake eblake(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachments:
- signature.asc (application/pgp-signature — 620 bytes)