Re: [libvirt] [PATCH] Fix error report from nl_recvmsg
On 02/28/2013 11:16 AM, Daniel P. Berrange wrote:
On Thu, Feb 28, 2013 at 11:11:53AM -0500, Laine Stump wrote:
> On 02/28/2013 08:37 AM, Daniel P. Berrange wrote:
>> From: "Daniel P. Berrange" <berrange(a)redhat.com>
>>
>> The nl_recvmsg does not always set errno. Instead it returns
>> its own custom set of error codes. Thus we were reporting the
>> wrong data.
>> ---
>> src/util/virnetlink.c | 5 +++--
>> 1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/util/virnetlink.c b/src/util/virnetlink.c
>> index 0b36fdc..8b47ede 100644
>> --- a/src/util/virnetlink.c
>> +++ b/src/util/virnetlink.c
>> @@ -335,8 +335,9 @@ virNetlinkEventCallback(int watch,
>> if (length == 0)
>> return;
>> if (length < 0) {
>> - virReportSystemError(errno,
>> - "%s", _("nl_recv returned with
error"));
>> + virReportError(VIR_ERR_INTERNAL_ERROR,
>> + _("nl_recv returned with error: %s"),
>> + nl_geterror(length));
> My recollection is that we specifically avoided calling nl_geterror()
> because it isn't threadsafe.
>
> I'll go take another look to verify.
I did check this, but only for libnl3 which merely does a static
string table lookup:
const char *nl_geterror(int error)
{
error = abs(error);
if (error > NLE_MAX)
error = NLE_FAILURE;
return errmsg[error];
}
nl_geterror() in libnl-1.1 calls strerror() which isn't threadsafe:
char *nl_geterror(void)
{
if (errbuf)
return errbuf;
if (nlerrno)
return strerror(nlerrno);
return "Sucess\n";
}
Of course strerror is only called from here if errbuf hasn't been set,
and I *think* that is rare, and we added a patch to all Fedora/RHEL
builds of libnl-1.1 that put errbuf and nlerrno into thread-local
storage quite a long time ago (August 2010:
https://bugzilla.redhat.com/show_bug.cgi?id=617291 ).
*But* the function that sets errbuf, __nl_error(), itself calls
strerror(). And when I look at strerror() in glibc, I'm not exactly
getting warm fuzzies about what could happen if it was called
simultaneously from two threads. It mallocs a global char* (after
freeing the original) then uses strerror_r to duplicate the standard
system error string into the malloc'ed buffer. So it's possible for one
thread to be dereferencing a pointer to a buffer that has already been
free'd by another thread.
Of course that's all academic, since __nl_error() is called when an
error occurs anyway, regardless of whether you subsequently decide to
call nl_geterror() or not.
So the conclusion is that I see no extra harm in calling nl_geterror(). ACK.