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@xxxxxxxxxx +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list