If defining a VM without any <seclabel> element present, the new verification code will crash as it deferences NULL, so we just need to skip over that. Second, if one does a roundtrip of a live VM config virsh dumpxml foo > foo.xml virsh define foo.xml and then stop and start the guest, it will now fail complaining that the security label is already defined. This is because the XML parser is mistakenly parsing the 'model' attribute even for VMs whose label is defined to be dynamic. This then exposed anothe bug in the qemudStartVMDaemon() method where it would not properly cleanup after a failed launch, failing to reset the dynamic allocated security label, and also failing to close a logfil file descriptor. This patch fixes all of these problems Daniel Index: src/domain_conf.c =================================================================== RCS file: /data/cvs/libvirt/src/domain_conf.c,v retrieving revision 1.74 diff -u -p -r1.74 domain_conf.c --- src/domain_conf.c 3 Apr 2009 10:55:51 -0000 1.74 +++ src/domain_conf.c 3 Apr 2009 12:14:54 -0000 @@ -1859,15 +1859,6 @@ virSecurityLabelDefParseXML(virConnectPt if (virXPathNode(conn, "./seclabel", ctxt) == NULL) return 0; - p = virXPathStringLimit(conn, "string(./seclabel/@model)", - VIR_SECURITY_MODEL_BUFLEN-1, ctxt); - if (p == NULL) { - virDomainReportError(conn, VIR_ERR_XML_ERROR, - "%s", _("missing security model")); - goto error; - } - def->seclabel.model = p; - p = virXPathStringLimit(conn, "string(./seclabel/@type)", VIR_SECURITY_LABEL_BUFLEN-1, ctxt); if (p == NULL) { @@ -1888,6 +1879,14 @@ virSecurityLabelDefParseXML(virConnectPt */ if (def->seclabel.type == VIR_DOMAIN_SECLABEL_STATIC || !(flags & VIR_DOMAIN_XML_INACTIVE)) { + p = virXPathStringLimit(conn, "string(./seclabel/@model)", + VIR_SECURITY_MODEL_BUFLEN-1, ctxt); + if (p == NULL) { + virDomainReportError(conn, VIR_ERR_XML_ERROR, + "%s", _("missing security model")); + goto error; + } + def->seclabel.model = p; p = virXPathStringLimit(conn, "string(./seclabel/label[1])", VIR_SECURITY_LABEL_BUFLEN-1, ctxt); @@ -1905,8 +1904,11 @@ virSecurityLabelDefParseXML(virConnectPt !(flags & VIR_DOMAIN_XML_INACTIVE)) { p = virXPathStringLimit(conn, "string(./seclabel/imagelabel[1])", VIR_SECURITY_LABEL_BUFLEN-1, ctxt); - if (p == NULL) + if (p == NULL) { + virDomainReportError(conn, VIR_ERR_XML_ERROR, + _("security imagelabel is missing")); goto error; + } def->seclabel.imagelabel = p; } Index: src/qemu_driver.c =================================================================== RCS file: /data/cvs/libvirt/src/qemu_driver.c,v retrieving revision 1.224 diff -u -p -r1.224 qemu_driver.c --- src/qemu_driver.c 3 Apr 2009 10:55:51 -0000 1.224 +++ src/qemu_driver.c 3 Apr 2009 12:14:54 -0000 @@ -332,7 +332,7 @@ qemudReconnectVMs(struct qemud_driver *d } if ((vm->logfile = qemudLogFD(NULL, driver->logDir, vm->def->name)) < 0) - return -1; + goto next_error; if (vm->def->id >= driver->nextvmid) driver->nextvmid = vm->def->id + 1; @@ -344,9 +344,12 @@ next_error: /* we failed to reconnect the vm so remove it's traces */ vm->def->id = -1; qemudRemoveDomainStatus(NULL, driver, vm); - virDomainDefFree(vm->def); - vm->def = vm->newDef; - vm->newDef = NULL; + /* Restore orignal def, if we'd loaded a live one */ + if (vm->newDef) { + virDomainDefFree(vm->def); + vm->def = vm->newDef; + vm->newDef = NULL; + } next: virDomainObjUnlock(vm); if (status) @@ -1319,14 +1322,6 @@ static int qemudStartVMDaemon(virConnect hookData.vm = vm; hookData.driver = driver; - /* If you are using a SecurityDriver with dynamic labelling, - then generate a security label for isolation */ - if (vm->def->seclabel.type == VIR_DOMAIN_SECLABEL_DYNAMIC && - driver->securityDriver && - driver->securityDriver->domainGenSecurityLabel && - driver->securityDriver->domainGenSecurityLabel(conn, vm) < 0) - return -1; - FD_ZERO(&keepfd); if (virDomainIsActive(vm)) { @@ -1335,6 +1330,14 @@ static int qemudStartVMDaemon(virConnect return -1; } + /* If you are using a SecurityDriver with dynamic labelling, + then generate a security label for isolation */ + if (vm->def->seclabel.type == VIR_DOMAIN_SECLABEL_DYNAMIC && + driver->securityDriver && + driver->securityDriver->domainGenSecurityLabel && + driver->securityDriver->domainGenSecurityLabel(conn, vm) < 0) + return -1; + if (vm->def->graphics && vm->def->graphics->type == VIR_DOMAIN_GRAPHICS_TYPE_VNC && vm->def->graphics->data.vnc.autoport) { @@ -1342,7 +1345,7 @@ static int qemudStartVMDaemon(virConnect if (port < 0) { qemudReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, "%s", _("Unable to find an unused VNC port")); - return -1; + goto cleanup; } vm->def->graphics->data.vnc.port = port; } @@ -1351,17 +1354,17 @@ static int qemudStartVMDaemon(virConnect virReportSystemError(conn, errno, _("cannot create log directory %s"), driver->logDir); - return -1; + goto cleanup; } if((vm->logfile = qemudLogFD(conn, driver->logDir, vm->def->name)) < 0) - return -1; + goto cleanup; emulator = vm->def->emulator; if (!emulator) emulator = virDomainDefDefaultEmulator(conn, vm->def, driver->caps); if (!emulator) - return -1; + goto cleanup; /* Make sure the binary we are about to try exec'ing exists. * Technically we could catch the exec() failure, but that's @@ -1371,7 +1374,7 @@ static int qemudStartVMDaemon(virConnect virReportSystemError(conn, errno, _("Cannot find QEMU binary %s"), emulator); - return -1; + goto cleanup; } if (qemudExtractVersionInfo(emulator, @@ -1380,21 +1383,17 @@ static int qemudStartVMDaemon(virConnect qemudReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, _("Cannot determine QEMU argv syntax %s"), emulator); - return -1; + goto cleanup; } if (qemuPrepareHostDevices(conn, vm->def) < 0) - return -1; + goto cleanup; vm->def->id = driver->nextvmid++; if (qemudBuildCommandLine(conn, driver, vm, qemuCmdFlags, &argv, &progenv, - &tapfds, &ntapfds, migrateFrom) < 0) { - close(vm->logfile); - vm->def->id = -1; - vm->logfile = -1; - return -1; - } + &tapfds, &ntapfds, migrateFrom) < 0) + goto cleanup; tmp = progenv; while (*tmp) { @@ -1457,12 +1456,8 @@ static int qemudStartVMDaemon(virConnect "%s", _("Unable to daemonize QEMU process")); ret = -1; } - } - - if (ret == 0) { vm->state = migrateFrom ? VIR_DOMAIN_PAUSED : VIR_DOMAIN_RUNNING; - } else - vm->def->id = -1; + } for (i = 0 ; argv[i] ; i++) VIR_FREE(argv[i]); @@ -1479,19 +1474,38 @@ static int qemudStartVMDaemon(virConnect VIR_FREE(tapfds); } - if (ret == 0) { - if ((qemudWaitForMonitor(conn, driver, vm, pos) < 0) || - (qemudDetectVcpuPIDs(conn, vm) < 0) || - (qemudInitCpus(conn, vm, migrateFrom) < 0) || - (qemudInitPasswords(conn, driver, vm) < 0) || - (qemudDomainSetMemoryBalloon(conn, vm, vm->def->memory) < 0) || - (qemudSaveDomainStatus(conn, qemu_driver, vm) < 0)) { - qemudShutdownVMDaemon(conn, driver, vm); - return -1; - } + if (ret == -1) + goto cleanup; + + if ((qemudWaitForMonitor(conn, driver, vm, pos) < 0) || + (qemudDetectVcpuPIDs(conn, vm) < 0) || + (qemudInitCpus(conn, vm, migrateFrom) < 0) || + (qemudInitPasswords(conn, driver, vm) < 0) || + (qemudDomainSetMemoryBalloon(conn, vm, vm->def->memory) < 0) || + (qemudSaveDomainStatus(conn, qemu_driver, vm) < 0)) { + qemudShutdownVMDaemon(conn, driver, vm); + ret = -1; + /* No need for 'goto cleanup' now since qemudShutdownVMDaemon does enough */ } return ret; + +cleanup: + if (vm->def->seclabel.type == VIR_DOMAIN_SECLABEL_DYNAMIC) { + VIR_FREE(vm->def->seclabel.model); + VIR_FREE(vm->def->seclabel.label); + VIR_FREE(vm->def->seclabel.imagelabel); + } + if (vm->def->graphics && + vm->def->graphics->type == VIR_DOMAIN_GRAPHICS_TYPE_VNC && + vm->def->graphics->data.vnc.autoport) + vm->def->graphics->data.vnc.port = -1; + if (vm->logfile != -1) { + close(vm->logfile); + vm->logfile = -1; + } + vm->def->id = -1; + return -1; } Index: src/security.c =================================================================== RCS file: /data/cvs/libvirt/src/security.c,v retrieving revision 1.3 diff -u -p -r1.3 security.c --- src/security.c 3 Apr 2009 10:55:51 -0000 1.3 +++ src/security.c 3 Apr 2009 12:14:54 -0000 @@ -33,7 +33,8 @@ virSecurityDriverVerify(virConnectPtr co unsigned int i; const virSecurityLabelDefPtr secdef = &def->seclabel; - if (STREQ(secdef->model, "none")) + if (!secdef->model || + STREQ(secdef->model, "none")) return 0; for (i = 0; security_drivers[i] != NULL ; i++) { @@ -42,7 +43,7 @@ virSecurityDriverVerify(virConnectPtr co } } virSecurityReportError(conn, VIR_ERR_XML_ERROR, - _("invalid security model")); + _("invalid security model '%s'"), secdef->model); return -1; } Index: src/security_selinux.c =================================================================== RCS file: /data/cvs/libvirt/src/security_selinux.c,v retrieving revision 1.5 diff -u -p -r1.5 security_selinux.c --- src/security_selinux.c 3 Apr 2009 10:55:51 -0000 1.5 +++ src/security_selinux.c 3 Apr 2009 12:14:54 -0000 @@ -162,11 +162,12 @@ SELinuxGenSecurityLabel(virConnectPtr co char *scontext = NULL; int c1 = 0; int c2 = 0; - if ( ( vm->def->seclabel.label ) || - ( vm->def->seclabel.model ) || - ( vm->def->seclabel.imagelabel )) { - virSecurityReportError(conn, VIR_ERR_ERROR, - "%s", _("security labellin already defined for VM")); + + if (vm->def->seclabel.label || + vm->def->seclabel.model || + vm->def->seclabel.imagelabel) { + virSecurityReportError(conn, VIR_ERR_INTERNAL_ERROR, + "%s", _("security label already defined for VM")); return rc; } @@ -197,7 +198,7 @@ SELinuxGenSecurityLabel(virConnectPtr co goto err; } vm->def->seclabel.model = strdup(SECURITY_SELINUX_NAME); - if (! vm->def->seclabel.model) { + if (!vm->def->seclabel.model) { virReportOOMError(conn); goto err; } -- |: 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 :|