On Fri, Sep 28, 2012 at 02:35:53PM +0200, Michael Hanselmann wrote:
> 2012/9/28 Iustin Pop <[email protected]>:
> > +-- | Prefixes a path with the current root directory
> > +addNodePrefix :: FilePath -> FilePath
> > +addNodePrefix path = do
> > + let val = unsafePerformIO $ getEnv "GANETI_ROOTDIR"
> > + case val of
> > + Just rootdir -> rootdir ++ path
> > + Nothing -> path
> > +
> >
> > And there are a few problems with it.
> >
> > Generally speaking, there are two ways to use unsafePerformIO:
> >
> > 1) however you want, if you are a wizard and know what you're doing
> > 2) by following a strict set of rules, otherwise
> >
> > […]
>
> Thanks for the explanation!
>
> > So in this case, the proper way to write this would be:
> >
> > {-# NOINLINE getRootDir #-}
> > getRootDir :: FilePath
> > getRootDir = unsafePerformIO $ getEnv "GANETI_ROOTDIR"
> >
> > so that this function/value is evaluated only once, and then use that in
> > addNodePrefix (which would be a honest pure function).
>
> Are there any guarantees it's only evaluated once?
100%? No. That why the code should, as much as possible, ensure that the
return value is indeed the same, even if evaluated multiple times. Now
granted this is not possible when talking about the environment… hence
why it's discouraged to use it in this context.
> > That was about unsafePerformIO. However, there are two other issues.
> > First, this does not catch any exceptions, and basically requires the
> > variable to be defined.
>
> System.Posix.Env.getEnv/getEnvDefault don't raise exceptions. The
> former returns a Maybe, the latter even takes a default value (only
> discovered this today).
Ah, I thought you were referring to System.Environment.getEnv.
> > Second, I'm not sure whether we actually need to use unsafePerformIO,
> > instead of making the node daemon log files functions that take the root
> > path with, reading the environment nicely in the daemon startup.
>
> I have to say this is beyond my Haskell abilities. I'm including an
> interdiff below which separates unsafePerformIO into a separate
> function and makes use of getEnvDefault. How about me committing this
> and you can then change it?
>
> --- a/htools/Ganeti/Path.hs
> +++ b/htools/Ganeti/Path.hs
> @@ -27,17 +27,16 @@ module Ganeti.Path where
>
> import qualified Ganeti.Constants as C
> import System.FilePath
> -import System.Posix.Env (getEnv)
> +import System.Posix.Env (getEnvDefault)
> import System.IO.Unsafe
>
> +{-# NOINLINE getRootDir #-}
> +getRootDir :: FilePath
> +getRootDir = unsafePerformIO $ getEnvDefault "GANETI_ROOTDIR" ""
>
> -- | Prefixes a path with the current root directory
> addNodePrefix :: FilePath -> FilePath
> -addNodePrefix path = do
> - let val = unsafePerformIO $ getEnv "GANETI_ROOTDIR"
> - case val of
> - Just rootdir -> rootdir ++ path
> - Nothing -> path
> +addNodePrefix path = getRootDir ++ path
Sounds good, are there no issues here with a potentially missing "/"?
(will path always start with a "/"?)
LGTM,
thanks.
