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

[John Ferlan]

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.