[libvirt] [PATCH] [OpenVZ] Segmentation fault

[Daniel P. Berrange]

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. 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

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/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|