[libvirt] [PATCH 0/3] virsh: Clean up and fix qemu-monitor-command
Mostly to allow piping of the prettified output into a file without having to fix it up later to stop syntax-check whining. Peter Krempa (3): virsh: qemu-monitor-command: Use macro for exclusive options virsh: qemu-monitor-command: Don't print extra newline with --pretty virsh: qemu-monitor-command: Simplify control flow tools/virsh-domain.c | 48 ++++++++++++++++++++---------------------------- 1 file changed, 20 insertions(+), 28 deletions(-) -- 2.9.2
--- tools/virsh-domain.c | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index dbdee5b..6c1bc2f 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -8945,6 +8945,8 @@ cmdQemuMonitorCommand(vshControl *ctl, const vshCmd *cmd) bool pad = false; virJSONValuePtr pretty = NULL; + VSH_EXCLUSIVE_OPTIONS("hmp", "pretty"); + dom = virshCommandOptDomain(ctl, cmd, NULL); if (dom == NULL) goto cleanup; @@ -8961,13 +8963,8 @@ cmdQemuMonitorCommand(vshControl *ctl, const vshCmd *cmd) } monitor_cmd = virBufferContentAndReset(&buf); - if (vshCommandOptBool(cmd, "hmp")) { - if (vshCommandOptBool(cmd, "pretty")) { - vshError(ctl, _("--hmp and --pretty are not compatible")); - goto cleanup; - } + if (vshCommandOptBool(cmd, "hmp")) flags |= VIR_DOMAIN_QEMU_MONITOR_COMMAND_HMP; - } if (virDomainQemuMonitorCommand(dom, monitor_cmd, &result, flags) < 0) goto cleanup; -- 2.9.2
The prettified JSON string already contains a newline so don't print another one. This allows to pipe the json output (in conjunction with the --quiet option) to files without having to truncate them afterwards. --- tools/virsh-domain.c | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 6c1bc2f..45fce76 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -8944,6 +8944,7 @@ cmdQemuMonitorCommand(vshControl *ctl, const vshCmd *cmd) virBuffer buf = VIR_BUFFER_INITIALIZER; bool pad = false; virJSONValuePtr pretty = NULL; + char *prettystr = NULL; VSH_EXCLUSIVE_OPTIONS("hmp", "pretty"); @@ -8969,22 +8970,20 @@ cmdQemuMonitorCommand(vshControl *ctl, const vshCmd *cmd) if (virDomainQemuMonitorCommand(dom, monitor_cmd, &result, flags) < 0) goto cleanup; - if (vshCommandOptBool(cmd, "pretty")) { - char *tmp; - pretty = virJSONValueFromString(result); - if (pretty && (tmp = virJSONValueToString(pretty, true))) { - VIR_FREE(result); - result = tmp; - } else { - vshResetLibvirtError(); - } + if (vshCommandOptBool(cmd, "pretty") && + (pretty = virJSONValueFromString(result)) && + (prettystr = virJSONValueToString(pretty, true))) { + vshPrint(ctl, "%s", prettystr); + } else { + vshResetLibvirtError(); + vshPrint(ctl, "%s\n", result); } - vshPrint(ctl, "%s\n", result); ret = true; cleanup: VIR_FREE(result); + VIR_FREE(prettystr); VIR_FREE(monitor_cmd); virJSONValueFree(pretty); if (dom) -- 2.9.2
On Mon, Aug 01, 2016 at 06:30:07AM +0200, Peter Krempa wrote:
The prettified JSON string already contains a newline so don't print another one. This allows to pipe the json output (in conjunction with the --quiet option) to files without having to truncate them afterwards. --- tools/virsh-domain.c | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-)
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 6c1bc2f..45fce76 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -8944,6 +8944,7 @@ cmdQemuMonitorCommand(vshControl *ctl, const vshCmd *cmd) virBuffer buf = VIR_BUFFER_INITIALIZER; bool pad = false; virJSONValuePtr pretty = NULL; + char *prettystr = NULL;
VSH_EXCLUSIVE_OPTIONS("hmp", "pretty");
@@ -8969,22 +8970,20 @@ cmdQemuMonitorCommand(vshControl *ctl, const vshCmd *cmd) if (virDomainQemuMonitorCommand(dom, monitor_cmd, &result, flags) < 0) goto cleanup;
- if (vshCommandOptBool(cmd, "pretty")) { - char *tmp; - pretty = virJSONValueFromString(result); - if (pretty && (tmp = virJSONValueToString(pretty, true))) { - VIR_FREE(result); - result = tmp; - } else { - vshResetLibvirtError(); - } + if (vshCommandOptBool(cmd, "pretty") && + (pretty = virJSONValueFromString(result)) &&
I find naming the variable pretty pretty confusing. virJSONValuePtr is in machine-friendly format and while it has the potential to be formatted as pretty, it is not pretty right now.
+ (prettystr = virJSONValueToString(pretty, true))) { + vshPrint(ctl, "%s", prettystr); + } else { + vshResetLibvirtError();
Resetting it even when --pretty was not specified feels strange. Also, grouping the if (--pretty) condition with the virJSONValue error checks makes the flow hard to read. Do we need the fallback? AFAIK there are only two issues possible when prettifying the string: * the system is out of memory possibly impossible, erroring out or even aborting virsh are acceptable here * virDomainQemuMonitorCommand did not return a JSON output For a domain with JSON monitor, the remote side calls virJSONValueToString, so this could only reasonably happen with a domain that only has HMP. If we error out on the --hmp --pretty combination, we can error out here too and do not need to fall back to raw output.
+ vshPrint(ctl, "%s\n", result); } - vshPrint(ctl, "%s\n", result);
There is also the option of keeping the \n in vshPrint and calling virTrimSpaces on the prettyfied output. Jan
On Mon, Aug 01, 2016 at 12:40:31 +0200, Ján Tomko wrote:
On Mon, Aug 01, 2016 at 06:30:07AM +0200, Peter Krempa wrote:
The prettified JSON string already contains a newline so don't print another one. This allows to pipe the json output (in conjunction with the --quiet option) to files without having to truncate them afterwards. --- tools/virsh-domain.c | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-)
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 6c1bc2f..45fce76 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -8944,6 +8944,7 @@ cmdQemuMonitorCommand(vshControl *ctl, const vshCmd *cmd) virBuffer buf = VIR_BUFFER_INITIALIZER; bool pad = false; virJSONValuePtr pretty = NULL; + char *prettystr = NULL;
VSH_EXCLUSIVE_OPTIONS("hmp", "pretty");
@@ -8969,22 +8970,20 @@ cmdQemuMonitorCommand(vshControl *ctl, const vshCmd *cmd) if (virDomainQemuMonitorCommand(dom, monitor_cmd, &result, flags) < 0) goto cleanup;
- if (vshCommandOptBool(cmd, "pretty")) { - char *tmp; - pretty = virJSONValueFromString(result); - if (pretty && (tmp = virJSONValueToString(pretty, true))) { - VIR_FREE(result); - result = tmp; - } else { - vshResetLibvirtError(); - } + if (vshCommandOptBool(cmd, "pretty") && + (pretty = virJSONValueFromString(result)) &&
I find naming the variable pretty pretty confusing. virJSONValuePtr is in machine-friendly format and while it has the potential to be formatted as pretty, it is not pretty right now.
I did not wan't to question the name choice of the previous autor thus I'm not changing it either.
+ (prettystr = virJSONValueToString(pretty, true))) { + vshPrint(ctl, "%s", prettystr); + } else { + vshResetLibvirtError();
Resetting it even when --pretty was not specified feels strange.
But certainly not wrong. In case no error was reported it's a no-op.
Also, grouping the if (--pretty) condition with the virJSONValue error checks makes the flow hard to read.
I agree on this point.
Do we need the fallback? AFAIK there are only two issues possible when prettifying the string: * the system is out of memory possibly impossible, erroring out or even aborting virsh are acceptable here * virDomainQemuMonitorCommand did not return a JSON output For a domain with JSON monitor, the remote side calls virJSONValueToString, so this could only reasonably happen with a domain that only has HMP. If we error out on the --hmp --pretty combination, we can error out here too and do not need to fall back to raw output.
Hmm, that's true. The slight change in semantics should not be an issue.
+ vshPrint(ctl, "%s\n", result); } - vshPrint(ctl, "%s\n", result);
There is also the option of keeping the \n in vshPrint and calling virTrimSpaces on the prettyfied output.
I thought you were into optimizing stuff rather than adding a pointless iteration through the string.
On Mon, Aug 01, 2016 at 13:19:40 +0200, Peter Krempa wrote:
On Mon, Aug 01, 2016 at 12:40:31 +0200, Ján Tomko wrote:
On Mon, Aug 01, 2016 at 06:30:07AM +0200, Peter Krempa wrote:
The prettified JSON string already contains a newline so don't print another one. This allows to pipe the json output (in conjunction with the --quiet option) to files without having to truncate them afterwards. --- tools/virsh-domain.c | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-)
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 6c1bc2f..45fce76 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -8944,6 +8944,7 @@ cmdQemuMonitorCommand(vshControl *ctl, const vshCmd *cmd) virBuffer buf = VIR_BUFFER_INITIALIZER; bool pad = false; virJSONValuePtr pretty = NULL; + char *prettystr = NULL;
VSH_EXCLUSIVE_OPTIONS("hmp", "pretty");
[...]
Do we need the fallback? AFAIK there are only two issues possible when prettifying the string: * the system is out of memory possibly impossible, erroring out or even aborting virsh are acceptable here * virDomainQemuMonitorCommand did not return a JSON output For a domain with JSON monitor, the remote side calls virJSONValueToString, so this could only reasonably happen with a domain that only has HMP. If we error out on the --hmp --pretty combination, we can error out here too and do not need to fall back to raw output.
Hmm, that's true. The slight change in semantics should not be an issue.
Actually I retract my statement. It's not true. If the VM uses just the HMP monitor then the conversation is done in HMP regardless of the flag and thus the fallback is still necessary.
The prettified JSON string already contains a newline so don't print another one. This allows to pipe the json output (in conjunction with the --quiet option) to files without having to truncate them afterwards. --- tools/virsh-domain.c | 1 + 1 file changed, 1 insertion(+) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 6c1bc2f..ff71193 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -8975,6 +8975,7 @@ cmdQemuMonitorCommand(vshControl *ctl, const vshCmd *cmd) if (pretty && (tmp = virJSONValueToString(pretty, true))) { VIR_FREE(result); result = tmp; + virTrimSpaces(result, NULL); } else { vshResetLibvirtError(); } -- 2.9.2
On Mon, Aug 01, 2016 at 01:34:56PM +0200, Peter Krempa wrote:
The prettified JSON string already contains a newline so don't print another one. This allows to pipe the json output (in conjunction with the --quiet option) to files without having to truncate them afterwards. --- tools/virsh-domain.c | 1 + 1 file changed, 1 insertion(+)
ACK Jan
Construct the query string by using virBufferTrim rather than having to remember to add a space and simplify cleanup path. --- tools/virsh-domain.c | 20 ++++++++------------ 1 file changed, 8 insertions(+), 12 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 45fce76..120b768 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -8942,22 +8942,19 @@ cmdQemuMonitorCommand(vshControl *ctl, const vshCmd *cmd) unsigned int flags = 0; const vshCmdOpt *opt = NULL; virBuffer buf = VIR_BUFFER_INITIALIZER; - bool pad = false; virJSONValuePtr pretty = NULL; char *prettystr = NULL; VSH_EXCLUSIVE_OPTIONS("hmp", "pretty"); - dom = virshCommandOptDomain(ctl, cmd, NULL); - if (dom == NULL) - goto cleanup; + if (!(dom = virshCommandOptDomain(ctl, cmd, NULL))) + return false; + + while ((opt = vshCommandOptArgv(ctl, cmd, opt))) + virBufferAsprintf(&buf, "%s ", opt->data); + + virBufferTrim(&buf, " ", -1); - while ((opt = vshCommandOptArgv(ctl, cmd, opt))) { - if (pad) - virBufferAddChar(&buf, ' '); - pad = true; - virBufferAdd(&buf, opt->data, -1); - } if (virBufferError(&buf)) { vshPrint(ctl, "%s", _("Failed to collect command")); goto cleanup; @@ -8986,8 +8983,7 @@ cmdQemuMonitorCommand(vshControl *ctl, const vshCmd *cmd) VIR_FREE(prettystr); VIR_FREE(monitor_cmd); virJSONValueFree(pretty); - if (dom) - virDomainFree(dom); + virDomainFree(dom); return ret; } -- 2.9.2
On Mon, Aug 01, 2016 at 06:30:08AM +0200, Peter Krempa wrote:
Construct the query string by using virBufferTrim rather than having to remember to add a space and simplify cleanup path. --- tools/virsh-domain.c | 20 ++++++++------------ 1 file changed, 8 insertions(+), 12 deletions(-)
ACK Jan
participants (2)
-
Ján Tomko -
Peter Krempa