On Thu, Feb 23, 2012 at 02:55:39PM +0100, Guido Trotter wrote: > On Tue, Feb 21, 2012 at 1:21 PM, Iustin Pop <[email protected]> wrote: > > +-- | All groups list. A bit hacking, as we can't enforce it's complete > > +-- at compile time. > > +allGroups :: [GanetiGroup] > > +allGroups = map DaemonGroup [minBound..maxBound] ++ > > + map ExtraGroup [minBound..maxBound] > > + > > Where do minBound and maxBound come from?
They are standard functions on the Bounded typeclass (http://hackage.haskell.org/packages/archive/base/latest/doc/html/Prelude.html#t:Bounded). They are pretty basic, and the idio above is simply a way to get all groups. Looking back at it, maybe we should make this a set or make sure the list only holds unique entries. > > +exceptionToResult :: IO a -> IO (Result a) > > +exceptionToResult value = do > > + result <- tryJust (\e -> if isDoesNotExistError e > > + then Nothing > > + else Just (show e)) value > > + case result of > > + Left e -> return $ Bad (show e) > > + Right v -> return $ Ok v > > + > > Seems to be a very specific exception to a very specific result. Would > it be possible to rename the function to something more specific? Right. Indeed, this is more like 'ignoreDoesNotExistErrors' or such. I'll send interdiff. > > +-- | Checks whether a daemon runs as the right user. > > +verifyDaemonUser :: GanetiDaemon -> RuntimeEnts -> IO () > > +verifyDaemonUser daemon ents = do > > + myuid <- getEffectiveUserID > > + case M.lookup daemon (fst ents) of > > + -- shouldn't happen, due to the above map construction > > + Nothing -> hPutStrLn stderr $ "Internal error: user entry for daemon " > > ++ > > + daemonName daemon ++ " not found" > > + > > Should we use M.! since we don't expect lookup to fail? That will > throw an error in case of failure, but this is an error condition > anyway, right? Yes; whether to use directly or try and handle a bit better the error is the same to me. > > + Just uid -> when (uid /= myuid) $ do > > + hPrintf stderr "%s started using wrong user ID (%d), \ > > + \expected %d\n" (daemonName daemon) > > + (fromIntegral myuid::Int) > > + (fromIntegral uid::Int) :: IO () > > + exitWith $ ExitFailure C.exitFailure > > Maybe we can abstract this to a different function? > assertUIDMatch myuid uid Sure! But I wouldn't call it assert, since it's not about internal condition; rather, checkUIDMatch. > > diff --git a/lib/daemon.py b/lib/daemon.py > > index 1e41b80..d5c12c7 100644 > > --- a/lib/daemon.py > > +++ b/lib/daemon.py > > @@ -621,6 +621,7 @@ def _VerifyDaemonUser(daemon_name): > > constants.NODED: getents.noded_uid, > > constants.CONFD: getents.confd_uid, > > } > > + assert daemon_name in daemon_uids, "Invalid daemon %s" % daemon_name > > > > This should go in independently of the haskell code, right? Yep. > > return (daemon_uids[daemon_name] == running_uid, running_uid, > > daemon_uids[daemon_name]) > > diff --git a/lib/runtime.py b/lib/runtime.py > > index 5180486..b2db408 100644 > > --- a/lib/runtime.py > > +++ b/lib/runtime.py > > @@ -171,7 +171,7 @@ def GetEnts(resolver=GetentResolver): > > """Singleton wrapper around resolver instance. > > > > As this method is accessed by multiple threads at the same time > > - we need to take thread-safty carefully > > + we need to take thread-safety carefully. > > > > Please fix this in some different patch! :) And yep. iustin
