On Mon, Nov 09, 2009 at 12:07:40PM +0100, Matthias Bolte wrote: > 2009/11/9 Daniel Veillard <veillard@xxxxxxxxxx>: > > On Sun, Nov 08, 2009 at 10:40:42PM +0100, Matthias Bolte wrote: > >> diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c > >> index c2c5a44..c5083cc 100644 > >> --- a/src/conf/node_device_conf.c > >> +++ b/src/conf/node_device_conf.c > >> @@ -1057,13 +1057,18 @@ virNodeDeviceDefParseXML(virConnectPtr conn, xmlXPathContextPtr ctxt, int create > >> /* Extract device name */ > >> if (create == EXISTING_DEVICE) { > >> def->name = virXPathString(conn, "string(./name[1])", ctxt); > >> + > >> + if (!def->name) { > >> + virNodeDeviceReportError(conn, VIR_ERR_NO_NAME, NULL); > >> + goto error; > >> + } > >> } else { > >> def->name = strdup("new device"); > >> - } > >> > >> - if (!def->name) { > >> - virNodeDeviceReportError(conn, VIR_ERR_NO_NAME, NULL); > >> - goto error; > >> + if (!def->name) { > >> + virReportOOMError(conn); > >> + goto error; > >> + } > >> } > >> > >> /* Extract device parent, if any */ > > > > I disagree with this one. The XPath string(./name[1]) can fail without > > this being an allocation error, it mays just be that there is no name > > element at the current node, and current behaviour looks better to me. > > Moreover if virXPathString() returns NULL because of a string allocation > > error it will already raise an OOMError > > I think you misread this one. The original code assigns the result of > virXPathString() or strdup() to def->name. After that it checks > def->name for NULL and reports an no-name error even if the NULL was > returned by strdup(), indicating an OOM error. Whoops, right :-) Rereading the patch it's fine ! > I moved the no-name error report into the virXPathString() case and > added an OOM error in the strdup() case. [...] > > So you had to add a filed in the iterator structure to report OOMs > > while running the iterator, nice work ! > > Well, DPB used this pattern in his "Convert virDomainObjListPtr to use > a hash of domain objects" patch, I just applied it here too :-) > > > ACK except for the one in virNodeDeviceDefParseXML(), very good job > > you must have spent a lot of time, thanks a lot ! > > > > Daniel > > > > It took some hours, but the task was simple: search and fix. Well thanks a lot ! and ACK :-) Danie -- 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