Re: [libvirt] [PATCH v2] buf: Fix possible infinite loop in EscapeString, VSnprintf

On 09/01/2010 03:41 PM, Cole Robinson wrote: Hmm, thinking about this a bit more, most callers blindly assume that if virBufferContentAndReset returns NULL, then they should report OOM. But in this case, OOM is a bit misleading if the real cause is that the format string was invalid and snprintf died with EINVAL (OOM implies good code but insufficient runtime resources; while EINVAL implies a programming error that should immediately be reported as a bug and fixed). Don't make any changes to your patch (it's fine as is: it fixes a real bug, and we are unlikely to hit the misleading OOM message since we shouldn't be passing bad format strings in the first place). But I'm wondering if a future patch should change buf->error to track the actual errno value of the first failure encountered (ENOMEM for the common case, but EILSEQ, EINVAL, EOVERFLOW for snprintf failures), and add a function like virBufferReportError() which calls the appropriate error message (virReportOOMError() vs. xxx(VIR_ERR_INTERNAL_ERROR, errno, "unexpected failure"). However, this would mean an audit of all existing virBuffer clients to call virBufferReportError() instead of virReportOOMError() on failure. Or indeed, if _all_ callers already (should) print errors, maybe it makes sense to inline the OOM error printing into virBufferContentAndReset in the first place. [I thought of this because the pending virCommand patch series has the same issue - not all failures are ENOMEM related, and it would be nice for virCommandRun to distinguish between the failures. But with the current state of those patches, the contract is that virCommandRun prints the error, rather than leaving it up to the caller.] -- Eric Blake eblake(a)redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org