'cmdQemuMonitorCommandQMPWrap' is checking whether the user provided
string is not valid JSON to avoid wrapping it. In cases where it's not
JSON we ignore the error and add the wrapper.

If the caller then reports a different non-libvirt error the error from
the JSON parsing would be printed as well. Reset errors we ignore:

 # virsh qemu-monitor-command cd --pass-fds a asdf
 error: Unable to parse FD number 'a'
 error: internal error: cannot parse json asdf: lexical error: invalid char in json text.
                                        asdf
                      (right here) ------^

In the above case 'asdf' is not valid JSON, but the code did wrap it
into '{"execute":"asdf"}', the only problem is the argument for
--pass-fds.

Signed-off-by: Peter Krempa <pkrempa@redhat.com>
---
 tools/virsh-domain.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 9b1b14cdc2..743660e794 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -9769,6 +9769,8 @@ cmdQemuMonitorCommandQMPWrap(vshControl *ctl,
     if (virJSONValueIsObject(fullcmdjson))
         return g_steal_pointer(&fullcmd);

+    vshResetLibvirtError();
+
     /* we try to wrap the command and possible arguments into a JSON object, if
      * we as fall back we pass through what we've got from the user */

@@ -9787,6 +9789,8 @@ cmdQemuMonitorCommandQMPWrap(vshControl *ctl,
          * JSON object wrapper and try using that */
         g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER;

+        vshResetLibvirtError();
+
         virBufferAddLit(&buf, "{");
         /* opt points to the _ARGV option bit containing the command so we'll
          * iterate through the arguments now */
-- 
2.35.1

On Tue, Mar 01, 2022 at 02:33:28PM +0100, Peter Krempa wrote:
>'cmdQemuMonitorCommandQMPWrap' is checking whether the user provided
>string is not valid JSON to avoid wrapping it. In cases where it's not
>JSON we ignore the error and add the wrapper.
>
>If the caller then reports a different non-libvirt error the error from
>the JSON parsing would be printed as well. Reset errors we ignore:
>
> # virsh qemu-monitor-command cd --pass-fds a asdf
> error: Unable to parse FD number 'a'
> error: internal error: cannot parse json asdf: lexical error: invalid char in json text.
>                                        asdf
>                      (right here) ------^
>
>In the above case 'asdf' is not valid JSON, but the code did wrap it
>into '{"execute":"asdf"}', the only problem is the argument for
>--pass-fds.
>
>Signed-off-by: Peter Krempa <pkrempa@redhat.com>
>---
> tools/virsh-domain.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
>diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
>index 9b1b14cdc2..743660e794 100644
>--- a/tools/virsh-domain.c
>+++ b/tools/virsh-domain.c
>@@ -9769,6 +9769,8 @@ cmdQemuMonitorCommandQMPWrap(vshControl *ctl,
>     if (virJSONValueIsObject(fullcmdjson))
>         return g_steal_pointer(&fullcmd);
>
>+    vshResetLibvirtError();
>+
>     /* we try to wrap the command and possible arguments into a JSON object, if
>      * we as fall back we pass through what we've got from the user */
>
>@@ -9787,6 +9789,8 @@ cmdQemuMonitorCommandQMPWrap(vshControl *ctl,
>          * JSON object wrapper and try using that */
>         g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER;
>
>+        vshResetLibvirtError();
>+

This is a little bit out of place and could be seen as confusing since
it is here just because the error could be set in a function that's in
an if condition.  I, for one, would vote for extracting that and making
it more clear why that is, but this fixes the immediate issue, so

Reviewed-by: Martin Kletzander <mkletzan@redhat.com>

>         virBufferAddLit(&buf, "{");
>         /* opt points to the _ARGV option bit containing the command so we'll
>          * iterate through the arguments now */
>-- 
>2.35.1
>

On Wed, Mar 02, 2022 at 09:39:24 +0100, Martin Kletzander wrote:
> On Tue, Mar 01, 2022 at 02:33:28PM +0100, Peter Krempa wrote:
> > 'cmdQemuMonitorCommandQMPWrap' is checking whether the user provided
> > string is not valid JSON to avoid wrapping it. In cases where it's not
> > JSON we ignore the error and add the wrapper.
> > 
> > If the caller then reports a different non-libvirt error the error from
> > the JSON parsing would be printed as well. Reset errors we ignore:
> > 
> > # virsh qemu-monitor-command cd --pass-fds a asdf
> > error: Unable to parse FD number 'a'
> > error: internal error: cannot parse json asdf: lexical error: invalid char in json text.
> >                                        asdf
> >                      (right here) ------^
> > 
> > In the above case 'asdf' is not valid JSON, but the code did wrap it
> > into '{"execute":"asdf"}', the only problem is the argument for
> > --pass-fds.
> > 
> > Signed-off-by: Peter Krempa <pkrempa@redhat.com>
> > ---
> > tools/virsh-domain.c | 4 ++++
> > 1 file changed, 4 insertions(+)
> > 
> > diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
> > index 9b1b14cdc2..743660e794 100644
> > --- a/tools/virsh-domain.c
> > +++ b/tools/virsh-domain.c
> > @@ -9769,6 +9769,8 @@ cmdQemuMonitorCommandQMPWrap(vshControl *ctl,
> >     if (virJSONValueIsObject(fullcmdjson))
> >         return g_steal_pointer(&fullcmd);
> > 
> > +    vshResetLibvirtError();
> > +
> >     /* we try to wrap the command and possible arguments into a JSON object, if
> >      * we as fall back we pass through what we've got from the user */
> > 
> > @@ -9787,6 +9789,8 @@ cmdQemuMonitorCommandQMPWrap(vshControl *ctl,
> >          * JSON object wrapper and try using that */
> >         g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER;
> > 
> > +        vshResetLibvirtError();
> > +
> 
> This is a little bit out of place and could be seen as confusing since
> it is here just because the error could be set in a function that's in
> an if condition.  I, for one, would vote for extracting that and making
> it more clear why that is, but this fixes the immediate issue, so

I've (trivially) re-structured the code so that vshResetLibvirtError
is done conditionally upon NULL return from virJSONValueFromString and
thus it makes it obvious what we are resetting.

> 
> Reviewed-by: Martin Kletzander <mkletzan@redhat.com>

Thanks!