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.