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

[John Ferlan]

On 11/23/2016 11:48 AM, Dawid Zamirski wrote:
> 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.


Seems our responses crossed paths...  Maybe it'd be best to send a v3...

A couple of other nits I noted were the 2nd-4th arguments to virClassNew
for vboxDriverOnceInit weren't placed under the "virClass" first
argument (off by a space or two)

Also the finger twister uninitialize was typed as unintialize in a
comment and in the commit message as unitialize

John
> 
> Regards,
> Dawid
>