[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



On Mon, Nov 09, 2009 at 12:07:40PM +0100, Matthias Bolte wrote:
> 2009/11/9 Daniel Veillard <veillard redhat com>:
> > 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 veillard com  | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library  http://libvirt.org/


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]