From: Francis Dupont <fdupont@isc.org>
Date: Tue, 9 Oct 2018 08:55:11 +0000 (+0200)
Subject: [153-netconf-fd-watcher] Addressed comments
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=6b0068b30cee56a90a10615eb65ee2dbdf6d0550;p=thirdparty%2Fkea.git

[153-netconf-fd-watcher] Addressed comments
---

diff --git a/src/bin/netconf/fd_watcher.cc b/src/bin/netconf/fd_watcher.cc
index 783e72d039..7e43ec02dc 100644
--- a/src/bin/netconf/fd_watcher.cc
+++ b/src/bin/netconf/fd_watcher.cc
@@ -10,6 +10,7 @@
 #include <config.h>
 
 #include <netconf/fd_watcher.h>
+#include <netconf/netconf_log.h>
 
 #include <boost/function.hpp>
 #include <sys/select.h>
@@ -197,8 +198,14 @@ FdWatcher::postHandler() {
             // sr_fd_event_process is C code.
             sr_fd_change_t* change_set = 0;
             size_t cnt = 0;
-            // ignore return because we don't know what to do on error.
-            sr_fd_event_process(fd, SR_FD_INPUT_READY, &change_set, &cnt);
+            int ret = sr_fd_event_process(fd, SR_FD_INPUT_READY,
+                                          &change_set, &cnt);
+            if (ret != SR_ERR_OK) {
+                // We don't know what to do on error...
+                // which BTW should never happen.
+                LOG_WARN(netconf_logger, NETCONF_FD_WATCHER_ERROR)
+                    .arg(string(sr_strerror(ret)));
+            }
             for (size_t i = 0; i < cnt; ++i) {
                 if (change_set[i].action == SR_FD_START_WATCHING) {
                     watcher->addFd(change_set[i].fd,
diff --git a/src/bin/netconf/fd_watcher.h b/src/bin/netconf/fd_watcher.h
index d4fe603022..8527e028cf 100644
--- a/src/bin/netconf/fd_watcher.h
+++ b/src/bin/netconf/fd_watcher.h
@@ -37,6 +37,9 @@ typedef boost::shared_ptr<isc::util::thread::Thread> ThreadPtr;
 /// The alternative is to leave sysrepo to create its own thread to
 /// handle suscriptions with an independent and likely incompatible
 /// event loop and of course locking issues.
+/// Reference:
+///     http://www.sysrepo.org/static/doc/html/subscribtions.html#callbacks
+///
 /// To simplify callbacks this class is implemented as a singleton.
 class FdWatcher : public boost::noncopyable {
 public:
@@ -59,10 +62,15 @@ public:
     int init(const asiolink::IOServicePtr& io_service);
 
     /// @brief Clear the fd watcher.
+    ///
+    /// Uses the terminate watch socket to wake up and terminate the thread
+    /// if it exists.
     void clear();
 
     /// @brief Add a file descriptor to watch.
     ///
+    /// The file descriptor is given by sysrepo for reading and/or writing.
+    ///
     /// @param fd The file descriptor to add.
     /// @param reading Boolean flag: true to watch for reading.
     /// @param writing Boolean flag: true to watch for writing.
@@ -70,6 +78,8 @@ public:
 
     /// @brief Delete a file descriptor to watch.
     ///
+    /// The file descriptor is given by sysrepo for reading and/or writing.
+    ///
     /// @param fd The file descriptor to delete.
     /// @param reading Boolean flag: true to watch for reading.
     /// @param writing Boolean flag: true to watch for writing.
@@ -78,13 +88,16 @@ public:
     /// @brief Post handler.
     ///
     /// The thread posts this handler on the IO service when there
-    /// should be a file descriptor available for IO.
+    /// should be a file descriptor available for IO. This handler
+    /// will be called by the process run loop in the main thread.
     static void postHandler();
 
     /// @brief Thread body.
     ///
     /// The thread body: select() on file descriptors, when one is
-    /// available posts fdAvailable and returns.
+    /// available posts fdAvailable and returns. As the file descriptors
+    /// include the terminate watch socket the thread can be triggered
+    /// from Kea too (vs only by sysrepo).
     static void threadBody();
 
     /// @brief Shared pointer to the IOService object where to post callbacks.
@@ -92,7 +105,7 @@ public:
 
     /// @brief Terminate watch socket.
     ///
-    /// Used to wake up the thread.
+    /// Used to wake up and terminate the thread from Kea.
     isc::util::WatchSocket watch_terminate_;
 
     /// @brief Polling thread.
diff --git a/src/bin/netconf/netconf_messages.mes b/src/bin/netconf/netconf_messages.mes
index ecc9fe77ec..a91278bb0b 100644
--- a/src/bin/netconf/netconf_messages.mes
+++ b/src/bin/netconf/netconf_messages.mes
@@ -20,6 +20,10 @@ in earlier log entries, possibly on lower levels.
 This is a fatal error message issued when the Netconf application
 encounters an unrecoverable error from within the event loop.
 
+% NETCONF_FD_WATCHER_ERROR sysrepo file descriptor watcher event process returned an error: %1
+This is a warning message when the sysrepo event process routine of the file
+descriptor watcher returns an error. This error is displayed and ignored.
+
 % NETCONF_RUN_EXIT application is exiting the event loop
 This is a debug message issued when the Netconf application exits its
 event loop.