[PATCH 0/5] be consistent about error checking xmlNodeGetContent() return

[Michal Privoznik]

On 7/20/20 10:48 PM, Laine Stump wrote:
> Awhile back I noticed that calls to xmlNodeGetContent() from libvirt
> code were inconsistent in their handling of the returned
> pointer. Sometimes we would assume the return was always non-NULL
> (dereferencing with wild abandon without concern for the
> consequences), sometimes we would interpret NULL as "", and sometimes
> as OOM. I sent mail about this to the list last week, wondering (and
> doubting) if we could assume that a NULL return would always mean OOM:
> 
> https://www.redhat.com/archives/libvir-list/2020-July/msg00333.html
> 
> After looking at the libxml code, danpb's determination was that a
> NULL return from xmlNodeGetContent *might* mean OOM, but it might also
> mean some odd XML that we weren't expecting, so we can't just always
> exit on a NULL return. He also agreed with (and expanded on) my
> suspicion that there really is no reliable way to tell the reason for
> a NULL return from xmlNodeGetContent, and suggested that possibly we
> could just examing the xmlNode directly to learn the content instead
> of calling xmlNodeGetContent().
> 
> This series is a followup to that discussion. The first 4 patches
> clean up the code with the result that:
> 
> 1) a libvirt wrapper function is always called instead of calling
> xmlNodeGetContent() directly.
> 
> 2) that wrapper function logs an error when it gets back NULL from
> xmlNodeGetContent().
> 
> 3) All the callers check for a NULL return, and do a "silent error
> return" themselves when there is a NULL.
> 
> In the final patch, I try out Dan's idea of looking at the xmlNode
> object directly to get the content. It turns out it's not as
> straightforward as you would think from just looking at the layout of
> the object - a full explanation is in patch 5. I'm mainly sending that
> patch as an "FYI" (one step back from an "RFC"), since really all it
> changes is that libvirt will exit on OOM, and log (different, but not
> any more informative) error messages when the problem isn't
> OOM. Unless someone has a strong opinion otherwise, I think just the
> first 4 patches should be applied, and users can just "deal" with the
> ambiguity in case of error.
> 
> 
> Laine Stump (5):
>    conf: refactor virDomainBlkioDeviceParseXML to reduce calls to
>      xmlNodeGetContent
>    util: replace all calls to xmlNodeGetContent with
>      virXMLNodeContentString
>    util: log an error if virXMLNodeContentString will return NULL
>    treat all NULL returns from virXMLNodeContentString() as an error
>    util: open code virXMLNodeContentString to access the node object
>      directly
> 
>   src/conf/domain_conf.c      | 194 ++++++++++++++++++++----------------
>   src/conf/network_conf.c     |   7 +-
>   src/conf/node_device_conf.c |   6 +-
>   src/util/virxml.c           |  53 +++++++++-
>   4 files changed, 169 insertions(+), 91 deletions(-)
> 

Reviewed-by: Michal Privoznik <mprivozn at redhat.com>

Michal