On Wed, Jan 30, 2013 at 11:45 +0100, Iustin Pop <[email protected]> wrote:
> On Tue, Jan 29, 2013 at 10:33:18PM +0000, Dato Simó wrote:
> > On Mon, Jan 28, 2013 at 11:55 +0100, Iustin Pop <[email protected]> wrote:
> > > On Thu, Jan 24, 2013 at 02:06:46AM +0000, Dato Simó wrote:
> > > > Parse auto-repair tags to set each instance in one of ArHealthy,
> > > > ArFailed,
> > > > or ArPendingRepair. The implementation tries to be well behaved when old
> > > > tags have been left behind, which future patches will still try _not_
> > > > to do.
> > > Hi,
> > > > +-- | Parse a tag into an 'AutoRepairData' record.
> > > > +--
> > > > +-- @Nothing@ is returned if the tag is not an auto-repair tag, or if
> > > > it's
> > > > +-- malformed.
> > > > +parseInitTag :: String -> Maybe AutoRepairData
> > > > +parseInitTag tag =
> > > > + let parsePending = do
> > > > + subtag <- chompPrefix C.autoRepairTagPending tag
> > > > + let [rtype, uuid, ts, jobs] = sepSplit ':' subtag
> > > This, and
> > > > + makeArData rtype uuid ts jobs
> > > > +
> > > > + parseResult = do
> > > > + subtag <- chompPrefix C.autoRepairTagResult tag
> > > > + let [rtype, uuid, ts, result, jobs] = sepSplit ':' subtag
> > > this seem very unsafe to me. While testing similar code in the Maybe
> > > monad on a test example with bad data, I get:
> > > runhaskell l.hs
> > > l.hs: l.hs:4:17-31: Irrefutable pattern failed for pattern [a, b, c]
> > > Does this actually behave correctly with invalid tags?
> > No, you're right. This is way too brittle for an "erroneous input"
> > condition.
> > How about a monadic wrapper for sepSplit? (If you think I better put it in
> > Utils.hs, just let me know.)
> > --- src/Ganeti/HTools/Program/Harep.hs
> > +++ src/Ganeti/HTools/Program/Harep.hs
> > @@ -85,12 +85,12 @@ parseInitTag :: String -> Maybe AutoRepairData
> > parseInitTag tag =
> > let parsePending = do
> > subtag <- chompPrefix C.autoRepairTagPending tag
> > - let [rtype, uuid, ts, jobs] = sepSplit ':' subtag
> > + [rtype, uuid, ts, jobs] <- splitN 4 subtag
> > makeArData rtype uuid ts jobs
> > parseResult = do
> > subtag <- chompPrefix C.autoRepairTagResult tag
> > - let [rtype, uuid, ts, result, jobs] = sepSplit ':' subtag
> > + [rtype, uuid, ts, result, jobs] <- splitN 5 subtag
> > arData <- makeArData rtype uuid ts jobs
> > result' <- autoRepairResultFromRaw result
> > return arData { arResult = Just result' }
> > @@ -106,6 +106,12 @@ parseInitTag tag =
> > , arResult = Nothing
> > , arTag = tag
> > }
> > +
> > + splitN n str = do
> > + let parts = sepSplit ':' str
> > + guard $ length parts == n
> > + return parts
> > +
> IMHO this is still ugly, as nothing in the *type system* prevents you
> from doing mistakes.
> What's wrong with simply:
> case sepSplit ':' subtag of
> [rtype, uuid, ts, result, jobs] -> return (rtype, uuid, ts, result, jobs)
> _ -> fail "Invalid tag contents"
> ? Yes, it is more verbose, but you have way more safety.
Actually, it reads quite nicely, thanks. Interdiff -b:
--- src/Ganeti/HTools/Program/Harep.hs
+++ src/Ganeti/HTools/Program/Harep.hs
@@ -85,15 +85,18 @@ parseInitTag :: String -> Maybe AutoRepairData
parseInitTag tag =
let parsePending = do
subtag <- chompPrefix C.autoRepairTagPending tag
- let [rtype, uuid, ts, jobs] = sepSplit ':' subtag
- makeArData rtype uuid ts jobs
+ case sepSplit ':' subtag of
+ [rtype, uuid, ts, jobs] -> makeArData rtype uuid ts jobs
+ _ -> fail ("Invalid tag: " ++ show tag)
parseResult = do
subtag <- chompPrefix C.autoRepairTagResult tag
- let [rtype, uuid, ts, result, jobs] = sepSplit ':' subtag
+ case sepSplit ':' subtag of
+ [rtype, uuid, ts, result, jobs] -> do
arData <- makeArData rtype uuid ts jobs
result' <- autoRepairResultFromRaw result
return arData { arResult = Just result' }
+ _ -> fail ("Invalid tag: " ++ show tag)
makeArData rtype uuid ts jobs = do
rtype' <- autoRepairTypeFromRaw rtype
> In principle we could even derive a splitN that returns tuples of the
> correct size via TH, if needed. But declaring a function that will
> supposedly return an N-length list is bad.
> You can use:
> let iniDataRes = mapM setInitialState $ Container.elems il
> iniDataChk <- exitIfBad "can't parse initial cluster state" iniDataRes
> This will format nicely the error and exit.
> > -- First step: check all pending repairs, see if they are completed.
> > + iniData <- iniDataChk
> (in any case, you can make iniData <- case iniDataRes of …, no need for
> first a let then a '<-').
Ah, neat, exitIfBad also unwraps/returns the value if it's Ok. Thanks.
In that case, simply:
@@ -443,7 +450,8 @@ main opts args = do
(ClusterData _ nl il _ _) <- loadExternalData opts'
- let iniData = map setInitialState $ Container.elems il
+ let iniDataRes = mapM setInitialState $ Container.elems il
+ iniData <- exitIfBad "when parsing auto-repair tags" iniDataRes
-- First step: check all pending repairs, see if they are completed.
iniData' <- bracket (L.getClient master) L.closeClient $
forM iniData . processPending
Cheers,
--
Dato Simó | [email protected]
Corp Fleet Management / Ganeti SRE (Dublin)
