[Bro-Dev] [Bro-Commits] [git/broctl] fastpath: Fix various bugs and remove some unused code (7108ea6)

[Robin Sommer]

There are two problems with this patch:

    - you're removing some parts from the plugin api that I think
      should stay. Just that it's not used currently doesn't mean it
      can't; that's what plugins are for. If it breaks the test plugin
      the bug is in there. And if somethign doesn't get called
      (cmd_restart_*) we should add the calls.

    - completedefault() is actually used, it's part of the Cmd API,
      see http://docs.python.org/2/library/cmd.html. Better to fix the
      method than to remove. 

So I've turned these changes into a branch for now
(topic/dnthayer/cleanup) and reverted the commit on fastpath.

Robin

On Wed, Jan 16, 2013 at 15:17 -0800, Daniel Thayer wrote:

> commit 7108ea62d2b91fcbffd66a6136cf94b9a05900b3
> Author: Daniel Thayer <dnthayer at illinois.edu>
> Date:   Wed Jan 16 17:11:50 2013 -0600
> 
>     Fix various bugs and remove some unused code
>     
>     Removed an unused extra parameter from the cmd_scripts_pre and
>     cmd_scripts_post methods (this was causing the TestPlugin.py to
>     crash broctl when running the "scripts" command).
>     
>     Removed an undefined "cleanup" command parameter "--keep-tmp" that the
>     "restart --clean" command was trying to use.
>     
>     The "status" command was not calling cmd_status_post (it was calling
>     cmd_status_pre twice).
>     
>     Removed the unused cmd_restart_pre and cmd_restart_post methods.
>     
>     Removed the unused function "completedefault" (it was using an
>     incorrect list of command names).
> 
> 
> >---------------------------------------------------------------
> 
> 7108ea62d2b91fcbffd66a6136cf94b9a05900b3
>  BroControl/plugin.py             |   80 ++------------------------------------
>  BroControl/plugins/TestPlugin.py |    8 ++--
>  bin/broctl.in                    |   19 +--------
>  3 files changed, 11 insertions(+), 96 deletions(-)
> 
> diff --git a/BroControl/plugin.py b/BroControl/plugin.py
> index ebbad72..b21ad62 100644
> --- a/BroControl/plugin.py
> +++ b/BroControl/plugin.py
> @@ -727,78 +727,6 @@ class Plugin(object):
>          pass
>  
>      @doc.api("override")
> -    def cmd_restart_pre(self, nodes, clean):
> -        """Called just before the ``restart`` command is run. It receives the
> -        list of nodes, and returns the list of nodes that should proceed with
> -        the command. *clean* is boolean indicating whether the ``--clean``
> -        argument has been given.
> -
> -        This method can be overridden by derived classes. The default
> -        implementation does nothing.
> -        """
> -        pass
> -
> -    @doc.api("override")
> -    def cmd_restart_post(self, results):
> -        """Called just after the ``restart`` command has finished. It receives
> -        the list of 2-tuples ``(node, bool)`` indicating the nodes the command
> -        was executed for, along with their success status. The remaining
> -        arguments are as with the ``pre`` method.
> -
> -        This method can be overridden by derived classes. The default
> -        implementation does nothing.
> -        """
> -        pass
> -
> -    @doc.api("override")
> -    def cmd_restart_pre(self, nodes, clean):
> -        """Called just before the ``restart`` command is run. It receives the
> -        list of nodes, and returns the list of nodes that should proceed with
> -        the command. *clean* is boolean indicating whether the ``--clean``
> -        argument has been given.
> -
> -        This method can be overridden by derived classes. The default
> -        implementation does nothing.
> -        """
> -        pass
> -
> -    @doc.api("override")
> -    def cmd_restart_post(self, results):
> -        """Called just after the ``restart`` command has finished. It receives
> -        the list of 2-tuples ``(node, bool)`` indicating the nodes the command
> -        was executed for, along with their success status. The remaining
> -        arguments are as with the ``pre`` method.
> -
> -        This method can be overridden by derived classes. The default
> -        implementation does nothing.
> -        """
> -        pass
> -
> -    @doc.api("override")
> -    def cmd_restart_pre(self, nodes, clean):
> -        """Called just before the ``restart`` command is run. It receives the
> -        list of nodes, and returns the list of nodes that should proceed with
> -        the command. *clean* is boolean indicating whether the ``--clean``
> -        argument has been given.
> -
> -        This method can be overridden by derived classes. The default
> -        implementation does nothing.
> -        """
> -        pass
> -
> -    @doc.api("override")
> -    def cmd_restart_post(self, results):
> -        """Called just after the ``restart`` command has finished. It receives
> -        the list of 2-tuples ``(node, bool)`` indicating the nodes the command
> -        was executed for, along with their success status. The remaining
> -        arguments are as with the ``pre`` method.
> -
> -        This method can be overridden by derived classes. The default
> -        implementation does nothing.
> -        """
> -        pass
> -
> -    @doc.api("override")
>      def cmd_cleanup_pre(self, nodes, all):
>          """Called just before the ``cleanup`` command is run. It receives the
>          list of nodes, and returns the list of nodes that should proceed with
> @@ -843,11 +771,11 @@ class Plugin(object):
>          pass
>  
>      @doc.api("override")
> -    def cmd_scripts_pre(self, nodes, full_path, check):
> +    def cmd_scripts_pre(self, nodes, check):
>          """Called just before the ``scripts`` command is run. It receives the
>          list of nodes, and returns the list of nodes that should proceed with
> -        the command. ``full_path`` and ``check`` are boolean indicating
> -        whether the ``-p`` and ``-c`` options were given, respectively.
> +        the command. *check* is boolean indicating whether the ``-c``
> +        option was given.
>  
>          This method can be overridden by derived classes. The default
>          implementation does nothing.
> @@ -855,7 +783,7 @@ class Plugin(object):
>          pass
>  
>      @doc.api("override")
> -    def cmd_scripts_post(self, nodes, full_path, check):
> +    def cmd_scripts_post(self, nodes, check):
>          """Called just after the ``scripts`` command has finished. Arguments
>          are as with the ``pre`` method.
>  
> diff --git a/BroControl/plugins/TestPlugin.py b/BroControl/plugins/TestPlugin.py
> index eae146e..74a4bd9 100644
> --- a/BroControl/plugins/TestPlugin.py
> +++ b/BroControl/plugins/TestPlugin.py
> @@ -190,11 +190,11 @@ class TestPlugin(BroControl.plugin.Plugin):
>      def cmd_capstats_post(self, nodes, interval):
>          self.message("TestPlugin: Test post 'capstats':  %s (%d)" % (self._nodes(nodes), interval))
>  
> -    def cmd_scripts_pre(self, nodes, full_path, check):
> -        self.message("TestPlugin: Test pre 'scripts':  %s (%s/%s)" % (self._nodes(nodes), full_path, check))
> +    def cmd_scripts_pre(self, nodes, check):
> +        self.message("TestPlugin: Test pre 'scripts':  %s (%s)" % (self._nodes(nodes), check))
>  
> -    def cmd_scripts_post(self, nodes, full_path, check):
> -        self.message("TestPlugin: Test post 'scripts': %s (%s/%s)" % (self._nodes(nodes), full_path, check))
> +    def cmd_scripts_post(self, nodes, check):
> +        self.message("TestPlugin: Test post 'scripts': %s (%s)" % (self._nodes(nodes), check))
>  
>      def cmd_print_pre(self, nodes, id):
>          self.message("TestPlugin: Test pre 'print':  %s (%s)" % (self._nodes(nodes), id))
> diff --git a/bin/broctl.in b/bin/broctl.in
> index fc8b163..d35e574 100755
> --- a/bin/broctl.in
> +++ b/bin/broctl.in
> @@ -251,8 +251,8 @@ class BroCtlCmdLoop(cmd.Cmd):
>                  # Can't delete the tmp here because log archival might still be
>                  # going on there in the background.
>                  util.output("cleaning up ...")
> -                self.do_cleanup("--keep-tmp " + args)
> -                self.postcmd(False, "--keep-tmp " + args)
> +                self.do_cleanup(args)
> +                self.postcmd(False, args)
>  
>                  if self.failed():
>                      return
> @@ -285,7 +285,7 @@ class BroCtlCmdLoop(cmd.Cmd):
>          if success:
>              nodes = plugin.Registry.cmdPreWithNodes("status", nodes)
>              control.status(nodes)
> -            plugin.Registry.cmdPreWithNodes("status", nodes)
> +            plugin.Registry.cmdPostWithNodes("status", nodes)
>  
>          return False
>  
> @@ -748,19 +748,6 @@ class BroCtlCmdLoop(cmd.Cmd):
>          success = control.processTrace(trace, options, scripts)
>          plugin.Registry.cmdPost("process", trace, options, scripts, success)
>  
> -    def completedefault(self, text, line, begidx, endidx):
> -        # Commands taken a "<nodes>" argument.
> -        nodes_cmds = ["check", "cleanup", "df", "diag", "restart", "start", "status", "stop", "top", "update", "attachgdb", "peerstatus", "list-scripts"],
> -
> -        args = line.split()
> -
> -        if not args or not args[0] in nodes_cmds:
> -            return []
> -
> -        nodes = ["manager", "workers", "proxies", "all"] + [n.name for n in Config.nodes()]
> -
> -        return [n for n in nodes if n.startswith(text)]
> -
>      # Prints the command's docstring in a form suitable for direct inclusion
>      # into the documentation.
>      def printReference(self):
> 
> _______________________________________________
> bro-commits mailing list
> bro-commits at bro-ids.org
> http://mailman.icsi.berkeley.edu/mailman/listinfo/bro-commits


-- 
Robin Sommer * Phone +1 (510) 722-6541 * robin at icir.org
ICSI/LBNL    * Fax   +1 (510) 666-2956 *   www.icir.org