[PATCH] cmdQemuMonitorCommandQMPWrap: Reset ignored errors from JSON parsing

[Peter Krempa]

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 at 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 at redhat.com>

Thanks!