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

On Wed, 2016-11-23 at 11:33 -0500, 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. > Sigh, it segfaults even in this case as well... :-( Calling VBOX_RELEASE(data->vboxObj) more than once will trigger a segfault on that call in both cases. The only way to address this would be to change _pfnUnitilize in src/vbox/vbox_tmpl.c to check: if (data->vboxObj) VBOX_RELEASE(data->vboxObj); if (data->vboxSession) VBOX_RELEASE(data->vboxSession); if (data->vboxClient) VBOX_RELEASE(data->vboxClient); only then it's safe to do this in src/vbox/vbox_common.c if (vboxSdkInitialize() < 0 || vboxExtractVersion() < 0) { gVBoxAPI.UPFN.Uninitialize(vbox_driver); virObjectUnref(vbox_driver); virMutexUnlock(&vbox_driver_lock); return NULL; } I can send v3 with those changes applied if needed.

Regards, Dawid