----- Original Message -----
On Sat, Dec 03, 2011 at 06:08:45PM -0500, Ayal Baron wrote:
----- Original Message -----
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 invented.)
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.py
_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 be 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 our current constants-aggregating approach, is that the number of files editted by autoconf is limitted.
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 into supervdsm.
That doesn't mean that supervdsm shouldn't have a canonical way of using it which could be later on leveraged in other places. i.e. assuming we converge on this way of doing things, it should apply to supervdsm as well.
I'm not sure what you are saying here... But I would like to stress again that "cat" is way too low-level and generic to be exposed by supervdsm. supervdsm should expose higher-level functions that cannot harm irrelevant parts of the host.
You are 100% correct. What I'm saying is that when *supervdsm* uses cat, it should do so like above. The API it exposes should not include a generic 'cat', no doubt about that.
Dan.