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

[Dawid Zamirski]

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