Re: [libvirt] [PATCH] Add missing OOM error checks, reports and cleanups

2009/11/9 Daniel Veillard <veillard(a)redhat.com&gt;: > 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.

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.