Re: [libvirt] [PATCH v2 1/2] vbox: change how vbox API is initialized.

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.