On Fri 02 Dec 2011 09:00:52 AM EST, Dan Kenigsberg wrote:
> On Thu, Dec 01, 2011 at 10:13:53PM -0500, Saggi Mizrahi wrote:
>> I want to suggest a different way of handling constants.py and config.py
>> the current way is clumsy and makes making convoluted code easy.
>> GLOBAL VARIABLES ARE BAD. It's not something I invented. It's well
>> accepted that proper scoping yields better code.
> FOLLOWING UPERCASE MAXIMS BLINDLY IS BAD. (and that's something I have
>> The way we use those two files shows how much of a mess globally
>> accessible variable can be.
>> Not putting paths to executable in constants.py
>> Instead we should have a module for each utility executable. The module
>> should wrap the utility with a pythonic interface. That way there is no
>> reason for more then one module to have access to a utility's path.
>> For example:
>> _CAT_PATH = @CAT_PATH@
>> def cat(files):
>> cmd = [_CAT_PATH] + files
>> out, err = exec(cmd)
>> return out
>> Now there is no need to know where cat is. You have a proper interface
>> to use. Preventing people from re-wrapping the utility over and over.
>> This in turn means you have a canonical wrapper that is more likely to work.
> I agree that our current behavior is clunky. Most external executable should
> wrapped by functions that exposes the functionality which we need from them.
> I am not sure that having a module per executable makes the most sense; I'd
> separate modularity based on Vdsm usage of them. Anyway, the only up side of
> current constants-aggregating approach, is that the number of files editted by
> autoconf is limitted.
I'm don't really care about how many utilities are wrapped per module.
I just want only one place to reference and actually prompt the utility
while all other places should use the wrapper.
> P.S. EXT_CAT is a horrible example. It exists only for letting Vdsm read
> /etc/multipath.conf. This functionality must be wrapped properly and shoved
cat was given as an example because it's the easiest utility to wrap I
could think of that is not echo. It was just used to illustrate the
>> Every other constant
>> Other constants should be moved to their proper modules and used only
>> when importing the module makes sense this will give you another step
>> where you make sure your decencies are correct.
>> Configuration files are just parameters to the program. They should be
>> treated as such. Their scope should not leave the main class of the
>> project. In our case VDSM is actually 2 projects meshed together like
>> Siamese twins so I think clientIf.py and hsm.py should be the only ones
>> with access to the config. Everything else should expect to get those
>> values in a regular programmatic fashion.
>> In my opinion these changes will make VDSM a lot more flexible, will
>> make more parts independent and easily separated and at that end help us
>> make VDSM better for generations to come.
> In my opinion our use of config.config is perfectly fine. The configuration
> is a single global object, and it makes sense to keep its Python
> as a single global object. I'd like keeping it this way because if Vm
> process needs a new parameter (say, when should we decide that migration is
> going anywhere) MigrationSourceThread can simply read it from the config. If
> take your approach, we would have to trickle the value all the way from
> through Vm down to the specific function that needs it. It would clutter the
> code and would not scale nicely with the number of configuration parameters.
The "I don't want to have to pass the parameter" excuse is always
thrown around for all sorts of bad practices (thread local storage
I would even stress that the pain with trickling down information helps
encourage developers to keep the system flat. And the less convoluted
your hierarchies are the simpler and more flexible the code is. In the
end, having things too accessible (ahm ahm sdc.produce()) usually
results in bad code.
You have a ton of patterns to mediate this effect.
"""configures the module. You see this a lot in the C\C++ world but
that doesn't mean it's bad""
def __init__(self, param):
"""Give values in the ctor."""
def configure(cls, param, param):
"""Less recommended but possible"""
def setDefaultSomething(self, value):
>> You can help!
>> * Start by taking you favorite utility and write a proper safe module
>> around it.
>> * When writing new code try and not access the configuration directly.
>> * Modules are free (really) don't just put stuff in misc\utils try and
>> put some order to things.
>> vdsm-devel mailing list
vdsm-devel mailing list