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@xxxxxxxxxx> Michal
