Re: [libvirt] RFC: Removing use of strerror() in favour of strerror_r()
Daniel P. Berrange 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,
const char *fmt, ...)
ATTRIBUTE_FORMAT(printf, 7, 8);
void virReportOOMFull(virConnectPtr conn,
int domcode,
const char *filename,
const char *funcname,
long long linenr);
And using a macro to automatically set domcode, filename, funcname
and linenr, since its tedious to make the caller supply all those
#define virReportSystemError(conn, theerrno, fmt,...) \
virReportSystemErrorFull((conn), \
VIR_FROM_THIS, \
(theerrno), \
__FILE__, __FUNCTION__, __LINE__, \
(fmt), __VA_ARGS__)
#define virReportOOM(conn) \
virReportOOMFull((conn), VIR_FROM_THIS, \
__FILE__, __FUNCTION__, __LINE__) \
So whereas before you might do
xenXMError (conn, VIR_ERR_INTERNAL_ERROR,
_("cannot stat %s: %s"),
filename, strerror(errno));
xenXMError (conn, VIR_ERR_NO_MEMORY, "%s", strerror(errno));
Now you would do:
virReportSystemError(conn, errno,
_("cannot stat: %s"),
filename);
virReportOOM(conn);
Before I actually go and start making this change across all 300 uses
of strerror(), do people agree with this approach....
Daniel
That seems like a good approach to me. In general, I think anything
that reduces the number of arguments that the caller must pass to
reporting fuctions is a good thing.
Dave