[libvirt] RFC: Removing use of strerror() in favour of strerror_r()

Dave Allan dallan at redhat.com
Thu Dec 11 18:22:19 UTC 2008


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




More information about the libvir-list mailing list