[libvirt] [PATCH] [OpenVZ] Segmentation fault

[Anton Protopopov]

2008/12/16 Daniel P. Berrange <berrange at redhat.com>

> On Tue, Dec 16, 2008 at 03:05:30PM +0300, Anton Protopopov wrote:
> > 2008/12/13 Ivan Vovk <ivovk at intermedia.net>
> >
> > >  Hello,
> > >
> > >
> > >
> > > after updating to the last official release 0.5.1 my application
> stopped
> > > working. No errors to the log file though i raised exceptions where it
> was
> > > possible.
> > >
> > > i checked with virsh xml description used for creating OpenVZ container
> and
> > > got the following:
> > >
> > >
> > >
> > > virsh # create ovz.xml
> > > Segmentation fault
> > >
> > > Attached patch, fixes that.
>

 - NACK. This patch  is a guarenteed deadlock.
+ this patch helps us to see a deadlock, which previously was invisible
because of segfault ;)


> You can't have one public
> driver API method, calling into another directly. openvzDomainSetVcpus()
> will attempt to lock the driver mutex, and the VM mutex, and then deadlock
> because openvzDomainCreateXML/openvzDomainDefineXML already hold the
> VM mutex.
>
> The way to address this is to move the backend logic out of the
> openvzDomainSetVcpus method, into a helper that is passed a pre-locked
> virDomainObjPtr instance. This helper can be called by public driver
> APIs openvzDomainSetVcpus and openvzDomainCreateXML  and
> openvzDomainDefineXML.
>
> eg, the helper would look like
>
>   static int openvzDomainSetVcpusInternal(virConnectPtr conn,
>                                           virDomainObjPtr vm)
>        char   str_vcpus[32];
>        const char *prog[] = { VZCTL, "--quiet", "set", PROGRAM_SENTINAL,
>                           "--cpus", str_vcpus, "--save", NULL };
>        pcpus = openvzGetNodeCPUs();
>        if (pcpus > 0 && pcpus < nvcpus)
>            nvcpus = pcpus;
>
>        snprintf(str_vcpus, 31, "%d", nvcpus);
>        str_vcpus[31] = '\0';
>
>        openvzSetProgramSentinal(prog, vm->def->name);
>        if (virRun(dom->conn, prog, NULL) < 0) {
>            openvzError(dom->conn, VIR_ERR_INTERNAL_ERROR,
>                        _("Could not exec %s"), VZCTL);
>            retur n-1;
>        }
>
>        vm->def->vcpus = nvcpus;
>        return 0;
>   }
>
> The public API would look like
>
>
> static int openvzDomainSetVcpus(virDomainPtr dom, unsigned int nvcpus) {
>    struct openvz_driver *driver = dom->conn->privateData;
>    virDomainObjPtr vm;
>    unsigned int pcpus;
>    int ret = -1;
>
>    openvzDriverLock(driver);
>    vm = virDomainFindByUUID(&driver->domains, dom->uuid);
>    openvzDriverUnlock(driver);
>    if (!vm) {
>        openvzError(dom->conn, VIR_ERR_INVALID_DOMAIN,
>                    "%s", _("no domain with matching uuid"));
>        goto cleanup;
>    }
>
>    if (nvcpus <= 0) {
>        openvzError(dom->conn, VIR_ERR_INTERNAL_ERROR,
>                    "%s", _("VCPUs should be >= 1"));
>        goto cleanup;
>    }
>
>    openvzDomainSetVcpusInternal(vm, nvcpus);
>
>    ret = 0;
>
> cleanup:
>    if (vm)
>        virDomainObjUnlock(vm);
>    return ret;
> }
>
>
> Similarly the Create/DefineXML methods calling openvzDomainSetVcpusInternal
>
Thanks, this seems to work (see attached patch).


>
> Regards,
> Daniel
> --
> |: Red Hat, Engineering, London   -o-   http://people.redhat.com/berrange/:|
> |: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org:|
> |: http://autobuild.org       -o-         http://search.cpan.org/~danberr/<http://search.cpan.org/%7Edanberr/>:|
> |: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505
> :|
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20081216/4b017071/attachment-0001.htm>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: libvirt_openvz_sigsegv+deadlock.patch
Type: application/octet-stream
Size: 4877 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20081216/4b017071/attachment-0001.obj>