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

[Daniel Veillard]

On Wed, Jun 03, 2009 at 05:42:20PM +0100, Daniel P. Berrange wrote:
> 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

  ACK, in case you didn't commit it yet !

Daniel

-- 
Daniel Veillard      | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
daniel at veillard.com  | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library  http://libvirt.org/