On Tue, Dec 16, 2008 at 03:05:30PM +0300, Anton Protopopov wrote: 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 :|

On Tue, Dec 16, 2008 at 04:34:15PM +0300, Anton Protopopov wrote: As a general rule the public virDomainPtr objects should not be passed down into internal methods. Likewise, when creating / defining a VM, the virGetDomain call should be the last one in the normal execution path of the method. Thus, just pass the 'virConnectPtr' instance to openvzDomainSetVcpusInternal as suggested above, and keep the setVcpusInternal calls above the call to virGetDomain. 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 :|

On Tue, Dec 16, 2008 at 05:11:10PM +0300, Anton Protopopov wrote: Thanks I've comited this patch 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 :|