[Kimchi-devel] [project-kimchi] [PATCH 1/4] Move configuration parsing to config.py

[Royce Lv]

On 2013年12月18日 18:54, Mark Wu wrote:
> On 12/18/2013 06:31 PM, Royce Lv wrote:
>> On 2013年12月18日 16:51, Mark Wu wrote:
>>> The configruation is also needed for other code except starting kimchi
>>> server. So it should be moved to a separate module, config.py. Then the
>>> configuration can be accessed directly by importing config module.
>>>
>>> Signed-off-by: Mark Wu <wudxw at linux.vnet.ibm.com>
>>> ---
>>>   src/kimchi/config.py.in | 23 ++++++++++++++++++++---
>>>   src/kimchid.in          | 20 +-------------------
>>>   2 files changed, 21 insertions(+), 22 deletions(-)
>>>
>>> diff --git a/src/kimchi/config.py.in b/src/kimchi/config.py.in
>>> index f3c408a..49d42db 100644
>>> --- a/src/kimchi/config.py.in
>>> +++ b/src/kimchi/config.py.in
>>> @@ -25,11 +25,9 @@
>>>   import libvirt
>>>   import os
>>>   import platform
>>> -
>>> -
>>> +from ConfigParser import SafeConfigParser
>>>   from glob import iglob
>>>
>>> -
>>>   from kimchi.xmlutils import xpath_get_text
>>>
>>>
>>> @@ -166,5 +164,24 @@ def get_plugin_tab_xml(name):
>>>       return os.path.join(_get_plugin_ui_dir(name), 
>>> 'config/tab-ext.xml')
>>>
>>>
>>> +CONFIG_FILE = "%s/kimchi.conf" % get_config_dir()
>>> +DEFAULT_LOG_LEVEL = "debug"
>>> +
>>> +config = SafeConfigParser()
>>> +config.add_section("server")
>>> +config.set("server", "host", "0.0.0.0")
>>> +config.set("server", "port", "8000")
>>> +config.set("server", "ssl_port", "8001")
>>> +config.set("server", "ssl_cert", "")
>>> +config.set("server", "ssl_key", "")
>>> +config.set("server", "environment", "development")
>>> +config.add_section("logging")
>>> +config.set("logging", "log_dir", get_default_log_dir())
>>> +config.set("logging", "log_level", DEFAULT_LOG_LEVEL)
>>> +
>>> +if os.path.exists(CONFIG_FILE):
>>> +    config.read(CONFIG_FILE)
>>> +
>>> +
>> I like explicit load rather than put these in top level and generate 
>> a global variable.
>> Think about two processes(let's say in websockify.py) both use this 
>> module, top level code executed for a second time, the later process 
>> may get a different config from the first.
>> But developer may fail to notice.
>> This will cause some trouble which is difficult to diagnose.
> Nope,  I don't think it will cause any race. First of all, no running 
> config happens outside that module.
> Second, that module will be imported only one time because they're 
> running in the same python process.
> So do you still think it can cause any problem?

1. This global variable need to be updated by OptionParser() to gurentee
the config is latest. global variable config in config.py in not
updated, if used elsewhere, it is the original value.
2. We can't gurantee we won't have process started by kimchi and use
config in the future. If others access this config else where, it is the
un-updated value.

>
>>>   if __name__ == '__main__':
>>>       print get_prefix()
>>> diff --git a/src/kimchid.in b/src/kimchid.in
>>> index 7865713..548fa52 100644
>>> --- a/src/kimchid.in
>>> +++ b/src/kimchid.in
>>> @@ -33,31 +33,13 @@ import kimchi.config
>>>   if kimchi.config.without_installation():
>>>       sys.path.append(kimchi.config.get_prefix())
>>>
>>> -from ConfigParser import SafeConfigParser
>>> +from kimchi.config import config
>>>   from optparse import OptionParser
>>>
>>>   ACCESS_LOG = "kimchi-access.log"
>>>   ERROR_LOG = "kimchi-error.log"
>>> -CONFIG_FILE = "%s/kimchi.conf" % kimchi.config.get_config_dir()
>>> -DEFAULT_LOG_DIR = kimchi.config.get_default_log_dir()
>>> -DEFAULT_LOG_LEVEL = "debug"
>>>
>>>   def main(options):
>>> -    config = SafeConfigParser()
>>> -    config.add_section("server")
>>> -    config.set("server", "host", "0.0.0.0")
>>> -    config.set("server", "port", "8000")
>>> -    config.set("server", "ssl_port", "8001")
>>> -    config.set("server", "ssl_cert", "")
>>> -    config.set("server", "ssl_key", "")
>>> -    config.set("server", "environment", "development")
>>> -    config.add_section("logging")
>>> -    config.set("logging", "log_dir", DEFAULT_LOG_DIR)
>>> -    config.set("logging", "log_level", DEFAULT_LOG_LEVEL)
>>> -
>>> -    if os.path.exists(CONFIG_FILE):
>>> -        config.read(CONFIG_FILE)
>>> -
>>>       host = config.get("server", "host")
>>>       port = config.get("server", "port")
>>>       ssl_port = config.get("server", "ssl_port")
>>
>