11:30 p.m.
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
11:30 p.m.
New subject: [libvirt] [PATCH 1/3] virsh: qemu-monitor-command: Use macro for exclusive options
---
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
11:30 p.m.
New subject: [libvirt] [PATCH 2/3] virsh: qemu-monitor-command: Don't print extra newline with --pretty
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
5:40 a.m.
New subject: [libvirt] [PATCH 2/3] virsh: qemu-monitor-command: Don't print extra newline with --pretty
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
6:19 a.m.
New subject: [libvirt] [PATCH 2/3] virsh: qemu-monitor-command: Don't print extra newline with --pretty
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.
6:24 a.m.
New subject: [libvirt] [PATCH 2/3] virsh: qemu-monitor-command: Don't print extra newline with --pretty
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.
6:34 a.m.
New subject: [libvirt] [PATCH v2 2/3] virsh: qemu-monitor-command: Don't print extra newline with --pretty
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
9:05 a.m.
New subject: [libvirt] [PATCH v2 2/3] virsh: qemu-monitor-command: Don't print extra newline with --pretty
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
11:30 p.m.
New subject: [libvirt] [PATCH 3/3] virsh: qemu-monitor-command: Simplify control flow
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
5:44 a.m.
New subject: [libvirt] [PATCH 3/3] virsh: qemu-monitor-command: Simplify control flow
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
3037
days inactive
3038
days old
10 comments
2 participants
participants (2)
-
Ján Tomko -
Peter Krempa