Re: [libvirt] [PATCHv5 08/23] blockjob: add qemu capabilities related to block jobs

On 04/17/2012 01:05 AM, Eric Blake wrote: Is this comment now incorrect? (You say the new qemu proposal doesn't allow interaction with "transaction") Should we be concerned about NULL string pointers for args that aren't qualified as ATTRIBUTE_NONNULL? 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? :-) So does this mean that all of our ATTRIBUTE_NONNULL declarations are pointless? > + qemuReportError(VIR_ERR_INVALID_ARG, "%s", > + _("monitor must not be NULL")); > + return -1; > + } > + > + if (mon->json) > + ret = qemuMonitorJSONDriveMirror(mon, device, file, format, flags); > + else > + qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", > + _("drive-mirror requires JSON monitor")); > + return ret; > +} > + > /* Use the transaction QMP command to run atomic snapshot commands. */ > int > qemuMonitorTransaction(qemuMonitorPtr mon, virJSONValuePtr actions) > @@ -2701,6 +2726,30 @@ qemuMonitorTransaction(qemuMonitorPtr mon, virJSONValuePtr actions) > return ret; > } > > +/* Use the drive-reopen monitor command. */ > +int > +qemuMonitorDriveReopen(qemuMonitorPtr mon, const char *device, > + const char *file, const char *format) > +{ > + int ret = -1; > + > + VIR_DEBUG("mon=%p, device=%s, file=%s, format=%s", > + mon, device, file, format); > + qemuReportError(VIR_ERR_INVALID_ARG, "%s", > + _("monitor must not be NULL")); > + return -1; > + } > + > + if (mon->json) > + ret = qemuMonitorJSONDriveReopen(mon, device, file, format); > + else > + qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", > + _("drive-reopen requires JSON monitor")); > + return ret; > +} > + > int qemuMonitorArbitraryCommand(qemuMonitorPtr mon, > const char *cmd, > char **reply, > diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h > index f3cdcdd..1b4b130 100644 > --- a/src/qemu/qemu_monitor.h > +++ b/src/qemu/qemu_monitor.h > @@ -510,6 +510,17 @@ int qemuMonitorDiskSnapshot(qemuMonitorPtr mon, > bool reuse); > int qemuMonitorTransaction(qemuMonitorPtr mon, virJSONValuePtr actions) > ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); > +int qemuMonitorDriveMirror(qemuMonitorPtr mon, > + const char *device, > + const char *file, > + const char *format, > + unsigned int flags) > + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3); > +int qemuMonitorDriveReopen(qemuMonitorPtr mon, > + const char *device, > + const char *file, > + const char *format) > + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3); > > int qemuMonitorArbitraryCommand(qemuMonitorPtr mon, > const char *cmd, > diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c > index eb58f13..e0ea505 100644 > --- a/src/qemu/qemu_monitor_json.c > +++ b/src/qemu/qemu_monitor_json.c > @@ -987,6 +987,10 @@ qemuMonitorJSONCheckCommands(qemuMonitorPtr mon, > qemuCapsSet(qemuCaps, QEMU_CAPS_BLOCKJOB_SYNC); > else if (STREQ(name, "block-job-cancel")) > qemuCapsSet(qemuCaps, QEMU_CAPS_BLOCKJOB_ASYNC); > + else if (STREQ(name, "drive-mirror")) > + qemuCapsSet(qemuCaps, QEMU_CAPS_DRIVE_MIRROR); > + else if (STREQ(name, "drive-reopen")) > + qemuCapsSet(qemuCaps, QEMU_CAPS_DRIVE_REOPEN); > } > > ret = 0; > @@ -3199,6 +3203,38 @@ cleanup: > } > > int > +qemuMonitorJSONDriveMirror(qemuMonitorPtr mon, > + const char *device, const char *file, > + const char *format, unsigned int flags) > +{ > + int ret = -1; > + virJSONValuePtr cmd; > + virJSONValuePtr reply = NULL; > + bool shallow = (flags & VIR_DOMAIN_BLOCK_REBASE_SHALLOW) != 0; > + bool reuse = (flags & VIR_DOMAIN_BLOCK_REBASE_REUSE_EXT) != 0; > + > + cmd = qemuMonitorJSONMakeCommand("drive-mirror", > + "s:device", device, > + "s:target", file, > + "b:full", !shallow, > + "s:mode", > + reuse ? "existing" : "absolute-paths", > + format ? "s:format" : NULL, format, > + NULL); > + if (!cmd) > + return -1; > + > + if ((ret = qemuMonitorJSONCommand(mon, cmd, &reply)) < 0) > + goto cleanup; > + ret = qemuMonitorJSONCheckError(cmd, reply); > + > +cleanup: > + virJSONValueFree(cmd); > + virJSONValueFree(reply); > + return ret; > +} > + > +int > qemuMonitorJSONTransaction(qemuMonitorPtr mon, virJSONValuePtr actions) > { > int ret = -1; > @@ -3226,6 +3262,33 @@ cleanup: > return ret; > } > > +int > +qemuMonitorJSONDriveReopen(qemuMonitorPtr mon, const char *device, > + const char *file, const char *format) > +{ > + int ret; > + virJSONValuePtr cmd; > + virJSONValuePtr reply = NULL; > + > + cmd = qemuMonitorJSONMakeCommand("drive-reopen", > + "s:device", device, > + "s:new-image-file", file, > + format ? "s:format" : NULL, format, > + NULL); > + if (!cmd) > + return -1; > + > + if ((ret = qemuMonitorJSONCommand(mon, cmd, &reply)) < 0) > + goto cleanup; > + > + ret = qemuMonitorJSONCheckError(cmd, reply); > + > +cleanup: > + virJSONValueFree(cmd); > + virJSONValueFree(reply); > + return ret; > +} > + > int qemuMonitorJSONArbitraryCommand(qemuMonitorPtr mon, > const char *cmd_str, > char **reply_str, > diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h > index aacbb5f..d93c37d 100644 > --- a/src/qemu/qemu_monitor_json.h > +++ b/src/qemu/qemu_monitor_json.h > @@ -230,8 +230,22 @@ int qemuMonitorJSONDiskSnapshot(qemuMonitorPtr mon, > const char *device, > const char *file, > const char *format, > - bool reuse); > -int qemuMonitorJSONTransaction(qemuMonitorPtr mon, virJSONValuePtr actions); > + bool reuse) > + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(3) > + ATTRIBUTE_NONNULL(4) ATTRIBUTE_NONNULL(5); > +int qemuMonitorJSONTransaction(qemuMonitorPtr mon, virJSONValuePtr actions) > + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); > +int qemuMonitorJSONDriveMirror(qemuMonitorPtr mon, > + const char *device, > + const char *file, > + const char *format, > + unsigned int flags) > + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3); > +int qemuMonitorJSONDriveReopen(qemuMonitorPtr mon, > + const char *device, > + const char *file, > + const char *format) > + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3); > > int qemuMonitorJSONArbitraryCommand(qemuMonitorPtr mon, > const char *cmd_str, 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...).