A problem encountered due to a bug in libpcap was reported to the caller as:

   An error occurred, but the cause is unknown

This was because the error had been logged in the DHCPSnoop
thread. The worker thread handling the API call to start a domain
spins up the DHCPSnoop thread which watches for dhcp packets with
libpcap, then uses virCondSignal() to notify the worker thread (which
has been waiting with virCondWait()). The worker thread knows that
there was an error (because threadStatus != THREAD_STATUS_OK), but the
error info had been stored in thread-specific storage for the other
thread, so the worker thread can only report that there was a failure,
but it doesn't know why.

The solution is to save the error that was logged (with
virErrorPreserveLast() into the object the is used to share info
between the threads, then we can set the error in the worker thread
using virErrorRestore().

In the case of the error I was looking at, this changed the "unknown"
message into:

    internal error: pcap_setfilter: can't remove kernel filter:
    Bad file descriptor

Signed-off-by: Laine Stump <laine@laine.org>
---
 src/nwfilter/nwfilter_dhcpsnoop.c | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/src/nwfilter/nwfilter_dhcpsnoop.c b/src/nwfilter/nwfilter_dhcpsnoop.c
index 8e955150fa..6069e70460 100644
--- a/src/nwfilter/nwfilter_dhcpsnoop.c
+++ b/src/nwfilter/nwfilter_dhcpsnoop.c
@@ -147,6 +147,7 @@ struct _virNWFilterSnoopReq {
     virNWFilterSnoopIPLeasePtr           start;
     virNWFilterSnoopIPLeasePtr           end;
     char                                *threadkey;
+    virErrorPtr                          threadError;
 
     virNWFilterSnoopThreadStatus         threadStatus;
     virCond                              threadStatusCond;
@@ -639,6 +640,7 @@ virNWFilterSnoopReqFree(virNWFilterSnoopReqPtr req)
 
     virMutexDestroy(&req->lock);
     virCondDestroy(&req->threadStatusCond);
+    virFreeError(req->threadError);
 
     VIR_FREE(req);
 }
@@ -1404,10 +1406,12 @@ virNWFilterDHCPSnoopThread(void *req0)
 
     /* let creator know how well we initialized */
     if (error || !threadkey || tmp < 0 || !worker ||
-        ifindex != req->ifindex)
+        ifindex != req->ifindex) {
+        virErrorPreserveLast(&req->threadError);
         req->threadStatus = THREAD_STATUS_FAIL;
-    else
+    } else {
         req->threadStatus = THREAD_STATUS_OK;
+    }
 
     virCondSignal(&req->threadStatusCond);
 
@@ -1713,9 +1717,16 @@ virNWFilterDHCPSnoopReq(virNWFilterTechDriverPtr techdriver,
     }
 
     /* sync with thread */
-    if (virCondWait(&req->threadStatusCond, &req->lock) < 0 ||
-        req->threadStatus != THREAD_STATUS_OK)
+    if (virCondWait(&req->threadStatusCond, &req->lock) < 0) {
+        virReportSystemError(errno, "%s",
+                             _("unable to wait on dhcp snoop thread"));
         goto exit_snoop_cancel;
+    }
+
+    if (req->threadStatus != THREAD_STATUS_OK) {
+        virErrorRestore(&req->threadError);
+        goto exit_snoop_cancel;
+    }
 
     virNWFilterSnoopReqUnlock(req);
 
-- 
2.14.3

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On 02/23/2018 04:05 PM, Laine Stump wrote:
> A problem encountered due to a bug in libpcap was reported to the caller as:
> 
>    An error occurred, but the cause is unknown
> 
> This was because the error had been logged in the DHCPSnoop
> thread. The worker thread handling the API call to start a domain
> spins up the DHCPSnoop thread which watches for dhcp packets with
> libpcap, then uses virCondSignal() to notify the worker thread (which
> has been waiting with virCondWait()). The worker thread knows that
> there was an error (because threadStatus != THREAD_STATUS_OK), but the
> error info had been stored in thread-specific storage for the other
> thread, so the worker thread can only report that there was a failure,
> but it doesn't know why.
> 
> The solution is to save the error that was logged (with
> virErrorPreserveLast() into the object the is used to share info
> between the threads, then we can set the error in the worker thread
> using virErrorRestore().
> 
> In the case of the error I was looking at, this changed the "unknown"
> message into:
> 
>     internal error: pcap_setfilter: can't remove kernel filter:
>     Bad file descriptor
> 
> Signed-off-by: Laine Stump <laine@laine.org>
> ---
>  src/nwfilter/nwfilter_dhcpsnoop.c | 19 +++++++++++++++----
>  1 file changed, 15 insertions(+), 4 deletions(-)

ACK and safe for freeze.

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list