"Daniel P. Berrange" <berrange@redhat.com> wrote:
The man page for 'strerror()' has this to say
The strerror() function returns a string describ- ing the error code passed in the argument errnum, possibly using the LC_MESSAGES part of the current locale to select the appropriate language. This string must not be modified by the application, but may be modified by a subsequent call to per- ror(3) or strerror(). No library function will modify this string.
ie, strerror is not even remotely safe to use if the application calling libvirt uses threads, regardless of whether libvirt itself is threaded.
The replacement is strerror_r(), but it is kind of tedious to use as you can't simply pass its return value straight in as format arg.
While looking at this I also noticed that we're rather inconsistent about whether we use VIR_ERR_INTERNAL_ERROR or VIR_ERR_SYSTEM_ERROR code for reporting system errors. So I'm intending to create a convenient function for reporting system errors that hides the sterrror_r horribleness and also standardizes on error code VIR_ERR_SYSTEM_ERROR. At the same time also creating a convenience function for reporting OOM, since we have wildly inconsistent reporting of that too.
The API contracts for src/virterror_internal.h are intended to be:
void virReportSystemErrorFull(virConnectPtr conn, int domcode, int theerrno, const char *filename, const char *funcname, long long linenr,
Can you use size_t instead? i.e., assuming linenr doesn't ever need to be negative.
const char *fmt, ...) ATTRIBUTE_FORMAT(printf, 7, 8);
void virReportOOMFull(virConnectPtr conn, int domcode, const char *filename, const char *funcname, long long linenr);
Same here.
Before I actually go and start making this change across all 300 uses of strerror(), do people agree with this approach....
Yes, definitely.