On Thu, Feb 23, 2012 at 04:39:15PM +0100, Iustin Pop wrote:
> 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.
OK, interdiff, except for the Python bits:
diff --git a/htools/Ganeti/Runtime.hs b/htools/Ganeti/Runtime.hs
index f9bb2ef..b6371d3 100644
--- a/htools/Ganeti/Runtime.hs
+++ b/htools/Ganeti/Runtime.hs
@@ -105,26 +105,24 @@ allGroups :: [GanetiGroup]
allGroups = map DaemonGroup [minBound..maxBound] ++
map ExtraGroup [minBound..maxBound]
-exceptionToResult :: IO a -> IO (Result a)
-exceptionToResult value = do
+ignoreDoesNotExistErrors :: IO a -> IO (Result a)
+ignoreDoesNotExistErrors 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
+ then Just (show e)
+ else Nothing) value
+ return $ eitherToResult result
-- | Computes the group/user maps.
getEnts :: IO (Result RuntimeEnts)
getEnts = do
users <- mapM (\daemon -> do
- entry <- exceptionToResult .
+ entry <- ignoreDoesNotExistErrors .
getUserEntryForName .
daemonUser $ daemon
return (entry >>= \e -> return (daemon, userID e))
) [minBound..maxBound]
groups <- mapM (\group -> do
- entry <- exceptionToResult .
+ entry <- ignoreDoesNotExistErrors .
getGroupEntryForName .
daemonGroup $ group
return (entry >>= \e -> return (group, groupID e))
@@ -141,14 +139,16 @@ getEnts = do
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"
-
- 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
+ -- note: we use directly ! as lookup failues shouldn't happen, due
+ -- to the above map construction
+ checkUidMatch (daemonName daemon) ((M.!) (fst ents) daemon) myuid
+
+-- | Check that two UIDs are matching or otherwise exit.
+checkUidMatch :: String -> UserID -> UserID -> IO ()
+checkUidMatch name expected actual =
+ when (expected /= actual) $ do
+ hPrintf stderr "%s started using wrong user ID (%d), \
+ \expected %d\n" name
+ (fromIntegral actual::Int)
+ (fromIntegral expected::Int) :: IO ()
+ exitWith $ ExitFailure C.exitFailure
--
iustin
