Opened 6 years ago

Closed 6 years ago

#9277 closed defect (fixed)

BridgeDB's handling of config files is non-persistent and uses an old-style class

Reported by: isis Owned by: isis
Priority: Medium Milestone:
Component: Circumvention/BridgeDB Version:
Severity: Keywords: bridgedb-0.1.0
Cc: isis Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

Every time the process receives a SIGHUP, the configuration is reset to the settings within bridgedb.conf, which does not allow for methods within BridgeDB to make persistent configuration changes.

The Main.Conf() class is also an old-style class which does not inherit from object(). This makes it difficult and buggy to subclass.

The Conf.getstate() and Conf.setstate() methods do not exist, as with most other special methods, due to being an old-style class. The previous two methods make serialization impossible.

Having proper support for congurable log levels in BridgeDB will require being able to make persistent config changes, otherwise the log level will just be continually reset to WARNING everytime a SIGHUP comes in.

Child Tickets

Change History (4)

comment:1 Changed 6 years ago by isis

Status: newneeds_review

comment:2 Changed 6 years ago by cypherpunks

Status: needs_reviewneeds_revision

On first pass this looks quite good.

  • (9e9ddc56e2f2bd98ff) I think the commits will be more comprehendible if each commit message describes what the commit does, instead of describing the patchset in the first commit (this may already be on your TODO list, though).
  • In Main.py:reconfigure(), can you add the type of configuration to the docstring?
  • I don't fully understand Conf.load(). I think the method is ok, but it's unique. Can you explain why the config file should "not ending with a 'c'"?
    +        elif isinstance(config_file, (ModuleType, self.__class__)):
    +            if ((hasattr(config_file, 'file') is not None) and
    +                not config_file.file.endswith('c')):
    +                    self.file = config_file.file 
    
  • Should this use self.file instead of config_file? (config.py:150, config.py:157)
    +        logging.info("Loading config file: %s" % config_file) 
    ...
    +            logging.err(err, "Loading config file '%s' failed!" % config_file) 
    

comment:3 in reply to:  2 Changed 6 years ago by isis

Status: needs_revisionneeds_review

Replying to cypherpunks:

On first pass this looks quite good.

Thanks for reviewing!

  • (9e9ddc56e2f2bd98ff) I think the commits will be more comprehendible if each commit message describes what the commit does, instead of describing the patchset in the first commit (this may already be on your TODO list, though).

Yes, I agree. The commit messages were originally that way, but because this branch and the logging branch were intertwined for #9199 and then split and then rebased again several times I got seriously annoyed with messing with git commit logs, decided it was a waste of my time, and copied what I wanted to say into a file, and then back into the first commit message on each branch.

  • In Main.py:reconfigure(), can you add the type of configuration to the docstring?

Sure thing.

* commit 9241c624aaf0d0220f3602fbbc9682a2bbc6a47c
| gpg: Signature made Thu 01 Aug 2013 00:00:38 UTC
gpg:                using RSA key A3ADB67A2CDB8B35
gpg: Good signature from "Isis! <isis@patternsinthevoid.net>" [ultimate]
gpg:                 aka "Isis <isis@leap.se>" [ultimate]
gpg:                 aka "Isis <isis@torproject.org>" [ultimate]
gpg:                 aka "Isis! <isis@riseup.net>" [ultimate]
gpg: Signature notation: isis@patternsinthevoid.net=0A6A58A14B5946ABDE18E207A3ADB67A2CDB8B35
gpg: Signature expires Fri 01 Aug 2014 00:00:38 UTC
Author:     Isis Lovecruft <isis@torproject.org>
| AuthorDate: 78 minutes ago
| Commit:     Isis Lovecruft <isis@torproject.org>
| CommitDate: 78 minutes ago
| 
|     Update bridgedb.Main.reconfigure docstring with configuration parameter type.
|     
|      * ADD :type: and :param: info on configuration kwarg for
|        bridgedb.Main.reconfigure() function.
| ---
|  lib/bridgedb/Main.py | 5 +++++
|  1 file changed, 5 insertions(+)
| 
| diff --git a/lib/bridgedb/Main.py b/lib/bridgedb/Main.py
| index fb94f2a..a0b89e5 100644
| --- a/lib/bridgedb/Main.py
| +++ b/lib/bridgedb/Main.py
| @@ -222,6 +222,11 @@ def reconfigure(configuration=None):
|      configuration file, and apply those settings, and then, if extra settings
|      were given in :attr:`bridgedb.config.TESTING_CONFIG`, apply those settings
|      on top of the settings from the configuration file.
| +
| +    :type configuration: :class:`bridgedb.config.Conf` or dict
| +    :param configuration: A previously created :class:`bridgedb.config.Conf`
| +        object, or a Python dictionary containing configuration key/value
| +        settings.
|      """
|      options, arguments = Opt.parseOpts()
|      settings = {}    
  • I don't fully understand Conf.load(). I think the method is ok, but it's unique. Can you explain why the config file should "not ending with a 'c'"?
    +        elif isinstance(config_file, (ModuleType, self.__class__)):
    +            if ((hasattr(config_file, 'file') is not None) and
    +                not config_file.file.endswith('c')):
    +                    self.file = config_file.file 
    

Added an explanation:

* commit 68c75cb06630677544e86408a6ebefbe4991dab9 (HEAD, tpo-isis/fix/9277-config, isislovecruft/fix/9277-config, greyarea/fix/9277-config, fix/9277-config)
| gpg: Signature made Thu 01 Aug 2013 00:09:15 UTC
gpg:                using RSA key A3ADB67A2CDB8B35
gpg: Good signature from "Isis! <isis@patternsinthevoid.net>" [ultimate]
gpg:                 aka "Isis <isis@leap.se>" [ultimate]
gpg:                 aka "Isis <isis@torproject.org>" [ultimate]
gpg:                 aka "Isis! <isis@riseup.net>" [ultimate]
gpg: Signature notation: isis@patternsinthevoid.net=0A6A58A14B5946ABDE18E207A3ADB67A2CDB8B35
gpg: Signature expires Fri 01 Aug 2014 00:09:15 UTC
Author:     Isis Lovecruft <isis@torproject.org>
| AuthorDate: 66 minutes ago
| Commit:     Isis Lovecruft <isis@torproject.org>
| CommitDate: 66 minutes ago
|
|     Add comment in bridgedb.config.Conf.load() explaining odd checks.
|
|      * Explain the `if not config_file.endswith('c') line of code, tl;dr: we don't
|        want to load compiled Python binary files, even if they otherwise seem like
|        a Python object.
| ---
|  lib/bridgedb/config.py | 6 ++++++
|  1 file changed, 6 insertions(+)
|
| diff --git a/lib/bridgedb/config.py b/lib/bridgedb/config.py
| index ebafe3a..34496ee 100644
| --- a/lib/bridgedb/config.py
| +++ b/lib/bridgedb/config.py
| @@ -143,6 +143,12 @@ class Conf(dict):
|              else:
|                  self.file = os.path.abspath(config_file)
|          elif isinstance(config_file, (ModuleType, self.__class__)):
| +            ## Because the config file is not technically a "flat file", but
| +            ## instead is a file containing Python global variables, when it
| +            ## is loaded it is treated as a Python source file. This means it
| +            ## gets compiled, and Python's idiot interpreter adds a 'c' to the
| +            ## extension of any compiled Python binary. We don't want to load
| +            ## binary files.
|              if ((hasattr(config_file, 'file') is not None) and
|                  not config_file.file.endswith('c')):
|                      self.file = config_file.file
  • Should this use self.file instead of config_file? (config.py:150, config.py:157)
    +        logging.info("Loading config file: %s" % config_file) 
    ...
    +            logging.err(err, "Loading config file '%s' failed!" % config_file) 
    

Effectively, they often end up being the same thing. When they are different, config_file is just self.file after the last '/':

(bridgedb)∃!isisⒶwintermute:(fix/9277-config *)~/code/torproject/bridgedb ∴ python ./lib/TorBridgeDB.py -t
> /home/isis/code/torproject/bridgedb/lib/bridgedb/config.py(159)load()
-> logging.info("Loading config file: %s" % config_file)
(Pdb) print config_file
./bridgedb.conf
(Pdb) print self.file
./bridgedb.conf

comment:4 Changed 6 years ago by isis

Keywords: bridgedb-0.1.0 added
Resolution: fixed
Status: needs_reviewclosed

This was resolved some time ago, see the bridgedb.persistent module.

Due to not using .rpy scripts within twisted.web.client.Session instances, keeping state within the HTTPServer module is currently difficult/impossible.

Note: See TracTickets for help on using tickets.