[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH] Fix error report from nl_recvmsg



On Thu, Feb 28, 2013 at 11:59:42AM -0500, Laine Stump wrote:
> 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 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.

Except the API signature is different, so my patch won't work with
both versions :-(

Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]