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@xxxxxxxxxxxx | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list