diff mbox series

[ovs-dev] vlog: Better handle syslog handler exceptions.

Message ID 20190225174336.22486-1-i.maximets@samsung.com
State Accepted
Headers show
Series [ovs-dev] vlog: Better handle syslog handler exceptions. | expand

Commit Message

Ilya Maximets Feb. 25, 2019, 5:43 p.m. UTC
'set_levels_from_string' doesn't check for exceptions that could
happen while opening syslog files or connecting to syslog sockets.

For example, if rsyslog stopped on a system:

  $ test-unixctl.py -vFACILITY:daemon --detach
  Traceback (most recent call last):
    File "../../../../tests/test-unixctl.py", line 90, in <module>
      main()
    File "../../../../tests/test-unixctl.py", line 61, in main
      ovs.vlog.handle_args(args)
    File "python/ovs/vlog.py", line 463, in handle_args
      msg = Vlog.set_levels_from_string(verbose)
    File "python/ovs/vlog.py", line 345, in set_levels_from_string
      Vlog.add_syslog_handler(words[1])
    File "python/ovs/vlog.py", line 321, in add_syslog_handler
      facility=syslog_facility)
    File "/python2.7/logging/handlers.py", line 759, in __init__
      self._connect_unixsocket(address)
    File "/python2.7/logging/handlers.py", line 787, in _connect_unixsocket
      self.socket.connect(address)
    File "/python2.7/socket.py", line 224, in meth
      return getattr(self._sock,name)(*args)
  socket.error: [Errno 111] Connection refused

In this case "/dev/log" file exists, so the check inside
'add_syslog_handler' doesn't help.

We need to catch the exceptions in 'set_levels_from_string' same way
as it done in 'init' function.
Also, we don't really need to check for '/dev/log' existence, because
exception will be catched on the upper layer and properly handled by
disabling the corresponding logger.

Fixes: d69d61c7c175 ("vlog: Ability to override the default log facility.")
Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
---
 python/ovs/vlog.py | 28 +++++++++++++++++-----------
 1 file changed, 17 insertions(+), 11 deletions(-)

Comments

Ben Pfaff Feb. 25, 2019, 7:02 p.m. UTC | #1
On Mon, Feb 25, 2019 at 08:43:36PM +0300, Ilya Maximets wrote:
> 'set_levels_from_string' doesn't check for exceptions that could
> happen while opening syslog files or connecting to syslog sockets.

Thanks, applied to master.

This could be considered a bug fix.  Let me know if you'd like it
backported.
Ilya Maximets Feb. 26, 2019, 1:12 p.m. UTC | #2
On 25.02.2019 22:02, Ben Pfaff wrote:
> On Mon, Feb 25, 2019 at 08:43:36PM +0300, Ilya Maximets wrote:
>> 'set_levels_from_string' doesn't check for exceptions that could
>> happen while opening syslog files or connecting to syslog sockets.
> 
> Thanks, applied to master.

Thanks.

> 
> This could be considered a bug fix.  Let me know if you'd like it
> backported.

Actually, I found this issue while trying to fix too slow TravisCI
testsuite jobs by disabling the syslog.

I backported this patch and sent it in a patch set that intended to
fix Travis builds on branch 2.10 and earlier:
  https://patchwork.ozlabs.org/project/openvswitch/list/?series=94247

Best regards, Ilya Maximets.
diff mbox series

Patch

diff --git a/python/ovs/vlog.py b/python/ovs/vlog.py
index 30a88891e..ae5156d60 100644
--- a/python/ovs/vlog.py
+++ b/python/ovs/vlog.py
@@ -237,7 +237,7 @@  class Vlog(object):
                     Vlog.__file_handler = logging.FileHandler(Vlog.__log_file)
                     logger.addHandler(Vlog.__file_handler)
             except (IOError, socket.error):
-                logger.setLevel(logging.CRITICAL)
+                logger.disabled = True
 
         ovs.unixctl.command_register("vlog/reopen", "", 0, 0,
                                      Vlog._unixctl_vlog_reopen, None)
@@ -305,22 +305,24 @@  class Vlog(object):
             return
 
         logger = logging.getLogger('syslog')
-        # Disable the logger if there is no infrastructure to support python
-        # syslog (to avoid repeated errors) or if the "null" syslog method
-        # requested by environment.
-        if (not os.path.exists("/dev/log")
-            or os.environ.get('OVS_SYSLOG_METHOD') == "null"):
+        # Disable the logger if the "null" syslog method requested
+        # by environment.
+        if os.environ.get('OVS_SYSLOG_METHOD') == "null":
             logger.disabled = True
             return
 
+        if facility is None:
+            facility = syslog_facility
+
+        new_handler = logging.handlers.SysLogHandler(address="/dev/log",
+                                                     facility=facility)
+
         if syslog_handler:
             logger.removeHandler(syslog_handler)
 
-        if facility:
-            syslog_facility = facility
+        syslog_handler = new_handler
+        syslog_facility = facility
 
-        syslog_handler = logging.handlers.SysLogHandler(address="/dev/log",
-                                                    facility=syslog_facility)
         logger.addHandler(syslog_handler)
         return
 
@@ -344,7 +346,11 @@  class Vlog(object):
                 return "Please supply a valid pattern and destination"
         elif words[0] == "FACILITY":
             if words[1] in FACILITIES:
-                Vlog.add_syslog_handler(words[1])
+                try:
+                    Vlog.add_syslog_handler(words[1])
+                except (IOError, socket.error):
+                    logger = logging.getLogger('syslog')
+                    logger.disabled = True
                 return
             else:
                 return "Facility %s is invalid" % words[1]