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