[libvirt] [PATCH 01/11] qemu: monitor: Extract handling of JSON block job error codes

[Peter Krempa]

On Wed, Apr 08, 2015 at 10:25:36 -0400, John Ferlan wrote:
> 
> 
> On 04/01/2015 01:20 PM, Peter Krempa wrote:
> > My intention is to split qemuMonitorJSONBlockJob() into simpler separate
> > functions for every block job type. Since the error handling code is the
> > same for all block jobs, this patch extracts the code into a separate
> > function that will later be reused in more places.
> > ---
> >  src/qemu/qemu_monitor_json.c | 64 +++++++++++++++++++++++++++-----------------
> >  1 file changed, 39 insertions(+), 25 deletions(-)
> > 
> > diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
> > index aa844ca..edd0a3f 100644
> > --- a/src/qemu/qemu_monitor_json.c
> > +++ b/src/qemu/qemu_monitor_json.c
> > @@ -4252,6 +4252,39 @@ qemuMonitorJSONBlockJobInfo(qemuMonitorPtr mon,
> >  }
> > 
> > 
> > +static int
> > +qemuMonitorJSONBlockJobError(virJSONValuePtr reply,
> > +                             const char *cmd_name,
> > +                             const char *device)
> > +{
> > +    if (!virJSONValueObjectHasKey(reply, "error"))
> > +        return 0;
> > +
> > +    if (qemuMonitorJSONHasError(reply, "DeviceNotActive")) {
> > +        virReportError(VIR_ERR_OPERATION_INVALID,
> > +                       _("No active operation on device: %s"), device);
> > +    } else if (qemuMonitorJSONHasError(reply, "DeviceInUse")) {
> > +        virReportError(VIR_ERR_OPERATION_FAILED,
> > +                       _("Device %s in use"), device);
> > +    } else if (qemuMonitorJSONHasError(reply, "NotSupported")) {
> > +        virReportError(VIR_ERR_OPERATION_INVALID,
> > +                       _("Operation is not supported for device: %s"), device);
> > +    } else if (qemuMonitorJSONHasError(reply, "CommandNotFound")) {
> > +        virReportError(VIR_ERR_OPERATION_INVALID,
> > +                       _("Command '%s' is not found"), cmd_name);
> > +    } else {
> > +        virJSONValuePtr error = virJSONValueObjectGet(reply, "error");
> 
> Because you touched it - Coverity whines that 'error' is not checked for
> NULL:
> 
> (8) Event returned_null: 	"virJSONValueObjectGet" returns null (checked
> 96 out of 99 times). [details]
> (16) Event var_assigned: 	Assigning: "error" = null return value from
> "virJSONValueObjectGet".
> Also see events:
> (17) Event dereference: 	Dereferencing a pointer that might be null
> "error" when calling "virJSONValueObjectGetString". [details]

I hate coverity for this. That's a false positive due to the fact that
we check right away that the reply object contains the error subobject.

> 
> 
> There's many examples (96) of checking...

Many examples, but here it's useless. The check will make it ugly. 

> 
> ACK with the check

I'll send a new version that will have different control flow. If we are
going to uglyfy this function for coverity, we might as well as fix it
so that the error object is not looked up several times which will then
make the coverity error disappear.

The downside will be that it will not be straight move of code.

Peter
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20150409/147b355d/attachment-0001.sig>