Re: [libvirt] PATCH: Fix redetection of transient QEMU VMs on daemon restarts

On Tue, Jun 02, 2009 at 03:55:27PM +0200, Daniel Veillard wrote: > On Wed, May 27, 2009 at 04:58:44PM +0100, Daniel P. Berrange wrote: > [...] > > + xmlXPathContextPtr ctxt) > > +{ > > + char *tmp = NULL; > > + long val; > > + xmlNodePtr config; > > + xmlNodePtr oldctxt; > > I would s/oldctxt/oldnode/ as what is saved is really only the old > XPath current node not the context itself. Good idea, changed this. > > [...] > > +char *virDomainObjFormat(virConnectPtr conn, > > + virDomainObjPtr obj, > > + int flags) > > +{ > > + char *config_xml = NULL, *xml = NULL; > > + virBuffer buf = VIR_BUFFER_INITIALIZER; > > + > > + virBufferVSprintf(&buf, "<domstatus state='%s' pid='%d'>\n", > > + virDomainStateTypeToString(obj->state), > > + obj->pid); > > + virBufferEscapeString(&buf, " <monitor path='%s'/>\n", obj->monitorpath); > > + > > + if (!(config_xml = virDomainDefFormat(conn, > > + obj->def, > > + flags))) > > Hum we are leaking the buffer content here. > > > + goto cleanup; > > + > > + virBufferAdd(&buf, config_xml, strlen(config_xml)); > > + virBufferAddLit(&buf, "</domstatus>\n"); > > + > > + xml = virBufferContentAndReset(&buf); > > +cleanup: > > + VIR_FREE(config_xml); > > + return xml; > > + > > +} Yes, and also forgetting to check virBufferError() to report OOM. Fixed the cleanup in this function now. > > + virDomainObjUnlock(obj); > > + virDomainReportError(conn, VIR_ERR_INTERNAL_ERROR, > > + _("unexpected domain %s already exists"), obj->def->name); > > > let's wrap to 80 columns > > [...] > > +/* > > + * Open an existing VM's monitor, and re-detect VCPUs > > + * threads > > maybe update the comment about the security labels too, especially as > this is a bit arcane. Updated these two. > > @@ -1519,10 +1519,8 @@ cleanup: > > vm->def->graphics[0]->type == VIR_DOMAIN_GRAPHICS_TYPE_VNC && > > vm->def->graphics[0]->data.vnc.autoport) > > vm->def->graphics[0]->data.vnc.port = -1; > > - if (vm->logfile != -1) { > > - close(vm->logfile); > > - vm->logfile = -1; > > - } > > + if (logfile != -1) > > + close(logfile); > > vm->def->id = -1; > > return -1; > > } > > Hum, do we still use vm->logfile field then ? Maybe I didn't see the > place where it was removed from the structure. Yep, this field in the struct has been killed off - see domain_conf.h Here's the updated patch in full