On Fri, Sep 21, 2012 at 05:15:42PM +0200, Iustin Pop wrote:
> On Fri, Sep 21, 2012 at 04:33:20PM +0200, Agata Murawska wrote:
> > On Fri, Sep 21, 2012 at 4:10 PM, Iustin Pop <[email protected]> wrote:
> > > On Fri, Sep 21, 2012 at 10:48:55AM +0200, Michael Hanselmann wrote:
> > >> unsafePerformIO is required to go from the IO monad to pure code.
> > >
> > > Short on time, but this is not the best way (just trying to prevent
> > > someone saying LGTM and committing this).
> >
> > You mean the fact that we do not sanitize the input that we read or
> > that we use something that is evil cheating mechanism that even has
> > "unsafe" in its name? ;)
>
> Neither, actually - or a little bit of both. 'unsafePerformIO' is not
> evil, but it must be used correctly.
Sorry for the late response. The code in question is:
+-- | 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
The key thing to understand about this function is not that it allows
you to drop "IO" from a type; but rather, that it moves the task of
ensuring type safety and other constraints (referentially transparency)
from the compiler to the user.
What kind of constraints? Well, if I have a function "foo :: String ->
String", the compiler guarantees that for a given x, "foo x" always has
the same result. If we add unsafePerformIO in the mix, it falls on _us_
to guarantee this. The addNodePrefix function above fails this test,
because the environment could change between calls, etc.
In order to avoid such cases, usually unsafePerformIO is used _only_
with initialisation functions, i.e. functions that have type "foo :: X",
which the compiler will then only call once (having no arguments, they
can't return different values, so it will evaluated only once), and
ensuring that the "unsafe-ness" is restricted to a single function.
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).
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.
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.
thanks,
iustin
