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

On Thu, Dec 11, 2008 at 05:34:34PM +0000, Daniel P. Berrange wrote:

Before I actually go and start making this change across all 300 uses of strerror(), do people agree with this approach....

yes that sounds a good way to clean this up ! Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/