On 11/23/2016 11:33 AM, Dawid Zamirski wrote:
On Wed, 2016-11-23 at 11:00 -0500, Dawid Zamirski wrote:
On Wed, 2016-11-23 at 08:55 -0500, John Ferlan wrote:
[...]
+ +static vboxDriverPtr +vboxGetDriverConnection(void) +{ + virMutexLock(&vbox_driver_lock); + + if (vbox_driver) { + virObjectRef(vbox_driver); + } else { + vbox_driver = vboxDriverObjNew(); + + if (!vbox_driver) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Failed to create vbox driver object.")); + return NULL; + } + } + + if (vboxSdkInitialize() < 0 || vboxExtractVersion() < 0) {
In this path should vboxSdkUninitialize be called (since it wouldn't be called in the destroy path)?
If vboxSdkUnintialize fails, VBoxSVC is not started so it does not need to be unintialized - which is in line with the sample code included SDK where it returns with EXIT_FAILUE in main if pfnClientInifialize fails. However, if vboxExtractVersion fails (unlikely) we might want to call gVBoxAPI.UPFN.Unintialize(vbox_driver) directly (not via vboxSdkUninitialize as it checks connectionCount > 0 which on failure would be 0). I've just tested both cases with gVBoxAPI.UPFN.Unintialize in the failure path, and calling it does not do any harm in both cases, so I guess it would be a good idea to put it there.
Actually, I was wrong in that it's safe to call gVBoxAPI.UPFN.Unintialize when vboxSdkInitialize fails - I got segfault when attempting to connect twice in VBOX_RELEASE(data->vboxObject). It's safe to do this though:
if (vboxSdkInitialize() < 0) { virObjectUnref(vbox_driver); virMutexUnlock(&vbox_driver_lock);
return NULL; }
if (vboxExtractVersion() < 0) { gVBoxAPI.UPFN.Uninitialize(vbox_driver); virObjectUnref(vbox_driver); virMutexUnlock(&vbox_driver_lock);
return NULL; }
i.e do not uninitalize on initialize failure, but do unintialize on vboxExractVersion failure.
Fair enough - what about the 4 places in vboxSdkInitialize that return failure after gVBoxAPI.UPFN.Initialize is successful? John FWIW: A closer look at vboxExtractVersion made me realize the "normal" fall through code path goes through the 'failed:' label, which looks odd.