On Fri, Mar 9, 2012 at 6:52 PM, Iustin Pop <[email protected]> wrote: > On Fri, Mar 09, 2012 at 03:00:18PM +0000, Guido Trotter wrote: >> On Tue, Feb 21, 2012 at 12:21 PM, Iustin Pop <[email protected]> wrote: >> >> > +-- | Builds link -> ip -> instname map. >> > +buildLinkIpInstnameMap :: ConfigData -> LinkIpMap >> > +buildLinkIpInstnameMap cfg = >> > + let cluster = configCluster cfg >> > + instances = M.elems . configInstances $ cfg >> > + defparams = (M.!) (clusterNicparams cluster) C.ppDefault >> > + nics = concatMap (\i -> [(instName i, nic) | nic <- instNics i]) >> > + instances >> > + in foldl' (\accum (iname, nic) -> >> > + let pparams = nicNicparams nic >> > + fparams = fillNICParams defparams pparams >> > + link = nicpLink fparams >> > + in case nicIp nic of >> > + Nothing -> accum >> > + Just ip -> let oldipmap = M.findWithDefault (M.empty) >> > + link accum >> > + newipmap = M.insert ip iname oldipmap >> > + in M.insert link newipmap accum >> > + ) M.empty nics >> >> So, looking some more at this function: >> >> - could we have a way to fetch the instances from the config with >> already filled params? This would definitely help simplifying this >> function. > > Yes, and indeed! > >> - maybe we can abstract the [instances] to [(nic, instance_name)] part? > > Yep. > >> - maybe we can have a function from instance (or nic, instance_name) >> to link -> (...) composed with a function from instance to ip -> >> instance_name to simplify it a bit > > Possible. Not sure. > >> Does this make sense? Shall I give it a try? > > Yes, it makes sense. But don't you want to wait until we commit these? > :) > > I'd prefer we commit what we have, and then iterate on it. Indeed, as > you say, these are very hackish, but I don't think we should iterate > out-of-tree until they are "very good"; rather, commit (but do not > enable hconfd yet) and then improve the code until we feel it's good > enough for release. >
Sounds good, but can we put at least a TODO on how we think we should improve that function to make it more readable? Thanks, Guido
