[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH 00/12] Various libxl driver improvements



John Ferlan wrote:
> On 09/03/2013 07:57 PM, Jim Fehlig wrote:
>   
>> Jim Fehlig wrote:
>>     
> <...snip...>
>   
>> I addressed the review comments from the various patches and pushed the
>> series.  Thanks for the reviews!
>>
>> Regards,
>> Jim
>>
>>     
>
> The changes have resulted in one Coverity found issue that seems to
> have been there "for a while" (at least as far as git blame is concerned).
>
> In libxlDomainCoreDump() Coverity has noted a FORWARD_NULL reference:
>
> 2004 	    if ((flags & VIR_DUMP_CRASH) && !vm->persistent) {
> 2005 	        virDomainObjListRemove(driver->domains, vm);
>
> (20) Event assign_zero: 	Assigning: "vm" = "NULL".
> Also see events: 	[var_deref_model]
>
> 2006 	        vm = NULL;
> 2007 	    }
> 2008 	
> 2009 	    ret = 0;
> 2010 	
> 2011 	cleanup_unpause:
>
> (21) Event var_deref_model: 	Passing null pointer "vm" to function "virDomainObjIsActive(virDomainObjPtr)", which dereferences it. [details]
> Also see events: 	[assign_zero]
>
> 2012 	    if (virDomainObjIsActive(vm) && paused) {
> 2013 	        if (libxl_domain_unpause(priv->ctx, dom->id) != 0) {
> 2014 	            virReportError(VIR_ERR_INTERNAL_ERROR,
> 2015 	                           _("After dumping core, failed to resume domain '%d' with"
>
>
> Not quite sure which is the best course of action, but it seems
> there needs to be at least a "if (vm && virDomainObjIsActive(vm)..."
>   

It took me a bit to figure out why the IsActive test is even needed, but
I think it is to handle !VIR_DUMP_LIVE and VIR_DUMP_CRASH.  I.e. we
don't want to unpause a domain we just crashed.  Checking for non-NULL
vm is certainly one way to fix the coverity warning.

> Also, the ListRemove call could probably be moved up into the
> previous if condition since they both require VIR_DUMP_CRASH
>   

Thanks for pointing that out.  What do you think of the attached patch?

Regards,
Jim
>From fe87e804478689b7cdf0ab417b459cb0d2f6118e Mon Sep 17 00:00:00 2001
From: Jim Fehlig <jfehlig suse com>
Date: Wed, 4 Sep 2013 14:27:20 -0600
Subject: [PATCH] libxl: Fix Coverity warning

John Ferlan reported the following Coverity warning:

In libxlDomainCoreDump() Coverity has noted a FORWARD_NULL reference:

2004 	    if ((flags & VIR_DUMP_CRASH) && !vm->persistent) {
2005 	        virDomainObjListRemove(driver->domains, vm);

(20) Event assign_zero: 	Assigning: "vm" = "NULL".
Also see events: 	[var_deref_model]

2006 	        vm = NULL;
2007 	    }
2008
2009 	    ret = 0;
2010
2011 	cleanup_unpause:

(21) Event var_deref_model: 	Passing null pointer "vm" to function
     "virDomainObjIsActive(virDomainObjPtr)", which dereferences it. [details]
Also see events: 	[assign_zero]

2012 	    if (virDomainObjIsActive(vm) && paused) {
2013 	        if (libxl_domain_unpause(priv->ctx, dom->id) != 0) {
2014 	            virReportError(VIR_ERR_INTERNAL_ERROR,

Removing the vm from domain obj list and setting it to NULL can be
done in the previous 'if (flags & VIR_DUMP_CRASH)' conditional.  Fix
the Coverity warning by ensuring vm is not NULL before testing if it
is still active.

Signed-off-by: Jim Fehlig <jfehlig suse com>
---
 src/libxl/libxl_driver.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
index 69b992b..426167c 100644
--- a/src/libxl/libxl_driver.c
+++ b/src/libxl/libxl_driver.c
@@ -1999,17 +1999,16 @@ libxlDomainCoreDump(virDomainPtr dom, const char *to, unsigned int flags)
 
         event = virDomainEventNewFromObj(vm, VIR_DOMAIN_EVENT_STOPPED,
                                          VIR_DOMAIN_EVENT_STOPPED_CRASHED);
-    }
-
-    if ((flags & VIR_DUMP_CRASH) && !vm->persistent) {
-        virDomainObjListRemove(driver->domains, vm);
-        vm = NULL;
+        if (!vm->persistent) {
+            virDomainObjListRemove(driver->domains, vm);
+            vm = NULL;
+        }
     }
 
     ret = 0;
 
 cleanup_unpause:
-    if (virDomainObjIsActive(vm) && paused) {
+    if (vm && virDomainObjIsActive(vm) && paused) {
         if (libxl_domain_unpause(priv->ctx, dom->id) != 0) {
             virReportError(VIR_ERR_INTERNAL_ERROR,
                            _("After dumping core, failed to resume domain '%d' with"
-- 
1.8.1.4


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]