On 09/01/2010 03:41 PM, Cole Robinson wrote:
+ size = buf->size - buf->use;
+ if ((count = vsnprintf(&buf->content[buf->use],
+ size, format, argptr))< 0) {
+ buf->error = 1;
+ goto err;
+ }
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@xxxxxxxxxx +1-801-349-2682
Libvirt virtualization library http://libvirt.org
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list