On Tue, Nov 20, 2012 at 11:26 AM, Iustin Pop <ius...@google.com> wrote:
> On Mon, Nov 19, 2012 at 03:19:51PM +0100, Michele Tartara wrote:
> > A new directory for haskell modules about block devices has been created
> > The parser is divided in two modules:
> > * one exports the data types describing the DRBD status
> > * one exports the parser itself
> >
> > Signed-off-by: Michele Tartara <mtart...@google.com>
> > ---
> > Makefile.am | 8 +
> > htools/Ganeti/Block/Drbd/Parser.hs | 333
> ++++++++++++++++++++++++++++++++++++
> > htools/Ganeti/Block/Drbd/Types.hs | 191 +++++++++++++++++++++
> > 3 files changed, 532 insertions(+), 0 deletions(-)
> > create mode 100644 htools/Ganeti/Block/Drbd/Parser.hs
> > create mode 100644 htools/Ganeti/Block/Drbd/Types.hs
>
> LGTM, I'll have a few small indentation fixes while pushing:
>
> > +-- | The parser for the version information lines.
> > +versionInfoParser :: Parser VersionInfo
> > +versionInfoParser =
> > + VersionInfo
> > + <$> optional versionP
> > + <*> optional apiP
> > + <*> optional protoP
> > + <*> optional srcVersion
> > + <*> (fmap unpack <$> optional gh)
> > + <*> (fmap unpack <$> optional builder)
> > + where versionP =
>
> Here "where" is indented right under '<*>'…
>
> > +-- | The parser for the connection state.
> > +connectionStateParser :: Parser ConnectionState
> > +connectionStateParser =
> > + standAlone
> > + <|> disconnecting
> > + <|> unconnected
> > + <|> timeout
> > + <|> brokenPipe
> > + <|> networkFailure
> > + <|> protocolError
> > + <|> tearDown
> > + <|> wfConnection
> > + <|> wfReportParams
> > + <|> connected
> > + <|> startingSyncS
> > + <|> startingSyncT
> > + <|> wfBitMapS
> > + <|> wfBitMapT
> > + <|> wfSyncUUID
> > + <|> syncSource
> > + <|> syncTarget
> > + <|> pausedSyncS
> > + <|> pausedSyncT
> > + <|> verifyS
> > + <|> verifyT
> > + <|> unconfigured
> > + where standAlone = A.string "StandAlone" *> pure StandAlone
> > + disconnecting = A.string "Disconnectiog" *> pure Disconnecting
> > + unconnected = A.string "Unconnected" *> pure Unconnected
> > + timeout = A.string "Timeout" *> pure Timeout
> > + brokenPipe = A.string "BrokenPipe" *> pure BrokenPipe
> > + networkFailure = A.string "NetworkFailure" *> pure
> NetworkFailure
> > + protocolError = A.string "ProtocolError" *> pure ProtocolError
> > + tearDown = A.string "TearDown" *> pure TearDown
> > + wfConnection = A.string "WFConnection" *> pure WFConnection
> > + wfReportParams = A.string "WFReportParams" *> pure
> WFReportParams
> > + connected = A.string "Connected" *> pure Connected
> > + startingSyncS = A.string "StartingSyncS" *> pure StartingSyncS
> > + startingSyncT = A.string "StartingSyncT" *> pure StartingSyncT
> > + wfBitMapS = A.string "WFBitMapS" *> pure WFBitMapS
> > + wfBitMapT = A.string "WFBitMapT" *> pure WFBitMapT
> > + wfSyncUUID = A.string "WFSyncUUID" *> pure WFSyncUUID
> > + syncSource = A.string "SyncSource" *> pure SyncSource
> > + syncTarget = A.string "SyncTarget" *> pure SyncTarget
> > + pausedSyncS = A.string "PausedSyncS" *> pure PausedSyncS
> > + pausedSyncT = A.string "PausedSyncT" *> pure PausedSyncT
> > + verifyS = A.string "VerifyS" *> pure VerifyS
> > + verifyT = A.string "VerifyT" *> pure VerifyT
> > + unconfigured = A.string "Unconfigured" *> pure Unconfigured
>
> FYI, for such block-style declarations we use a different style usually:
>
> disconnecting = A.string "Disconnectiog" *> pure Disconnecting
> unconnected = A.string "Unconnected" *> pure Unconnected
> timeout = A.string "Timeout" *> pure Timeout
> brokenPipe = A.string "BrokenPipe" *> pure BrokenPipe
> networkFailure = A.string "NetworkFailure" *> pure NetworkFailure
> protocolError = A.string "ProtocolError" *> pure ProtocolError
> tearDown = A.string "TearDown" *> pure TearDown
> wfConnection = A.string "WFConnection" *> pure WFConnection
> wfReportParams = A.string "WFReportParams" *> pure WFReportParams
>
> aligned on both the '=' and the '*>'. IMHO it results in _much_ better
> readability.
>
> > +-- | Parser for recognizing strings describing two elements of the same
> type
> > +-- separated by a '/'. The first one is considered local, the second
> remote.
> > +localRemoteParser :: Parser a -> Parser (LocalRemote a)
> > +localRemoteParser parser = LocalRemote <$> parser <*> (A.char '/' *>
> parser)
> > +
> > +-- | The parser for resource roles.
> > +roleParser :: Parser Role
> > +roleParser =
> > + primary
> > + <|> secondary
> > + <|> unknown
> > + where primary = A.string "Primary" *> pure Primary
> > + secondary = A.string "Secondary" *> pure Secondary
> > + unknown = A.string "Unknown" *> pure Unknown
>
> Whereas here "where" is much more indented. I'd expect it around the
> same level, somewhere under <|>.
>
> > +-- | The parser for disk states.
> > +diskStateParser :: Parser DiskState
> > +diskStateParser =
> > + diskless
> > + <|> attaching
> > + <|> failed
> > + <|> negotiating
> > + <|> inconsistent
> > + <|> outdated
> > + <|> dUnknown
> > + <|> consistent
> > + <|> upToDate
> > + where diskless = A.string "Diskless" *> pure Diskless
> > + attaching = A.string "Attaching" *> pure Attaching
> > + failed = A.string "Failed" *> pure Failed
> > + negotiating = A.string "Negotiating" *> pure Negotiating
> > + inconsistent = A.string "Inconsistent" *> pure Inconsistent
> > + outdated = A.string "Outdated" *> pure Outdated
> > + dUnknown = A.string "DUnknown" *> pure DUnknown
> > + consistent = A.string "Consistent" *> pure Consistent
> > + upToDate = A.string "UpToDate" *> pure UpToDate
> > +
> > +-- | The parser for I/O flags.
> > +ioFlagsParser :: Parser String
> > +ioFlagsParser = fmap unpack . A.takeWhile $ not . isBadEndOfLine
> > +
> > +-- | The parser for performance indicators.
> > +performanceIndicatorsParser :: Parser PerformanceIndicators
> > +performanceIndicatorsParser =
> > + PerformanceIndicators
> > + <$> skipSpacesAndString "ns:" A.decimal
> > + <*> skipSpacesAndString "nr:" A.decimal
> > + <*> skipSpacesAndString "dw:" A.decimal
> > + <*> skipSpacesAndString "dr:" A.decimal
> > + <*> skipSpacesAndString "al:" A.decimal
> > + <*> skipSpacesAndString "bm:" A.decimal
> > + <*> skipSpacesAndString "lo:" A.decimal
> > + <*> skipSpacesAndString "pe:" A.decimal
> > + <*> skipSpacesAndString "ua:" A.decimal
> > + <*> skipSpacesAndString "ap:" A.decimal
> > + <*> optional (skipSpacesAndString "ep:" A.decimal)
> > + <*> optional (skipSpacesAndString "wo:" A.anyChar)
> > + <*> optional (skipSpacesAndString "oos:" A.decimal)
> > + <* skipSpaces <* A.endOfLine
> > +
> > +-- | The parser for the syncronization status.
> > +syncStatusParser :: Parser SyncStatus
> > +syncStatusParser = do
> > + _ <- statusBarParser
> > + percent <-
> > + skipSpacesAndString "sync'ed:" $ skipSpaces *> A.double <* A.char
> '%'
> > + partSyncSize <- skipSpaces *> A.char '(' *> A.decimal
> > + totSyncSize <- A.char '/' *> A.decimal <* A.char ')'
> > + sizeUnit <- sizeUnitParser <* optional A.endOfLine
> > + timeToEnd <- skipSpacesAndString "finish:" $ skipSpaces *> timeParser
> > + sp <-
> > + skipSpacesAndString "speed:" $
> > + skipSpaces
> > + *> commaIntParser
> > + <* skipSpaces
> > + <* A.char '('
> > + <* commaIntParser
> > + <* A.char ')'
> > + w <- skipSpacesAndString "want:" (
> > + skipSpaces
> > + *> (Just <$> commaIntParser)
> > + )
> > + <|> pure Nothing
> > + sSizeUnit <- skipSpaces *> sizeUnitParser
> > + sTimeUnit <- A.char '/' *> timeUnitParser
> > + _ <- A.endOfLine
> > + return $
> > + SyncStatus percent partSyncSize totSyncSize sizeUnit timeToEnd sp w
> > + sSizeUnit sTimeUnit
> > +
> > +-- | The parser for recognizing (and discarding) the sync status bar.
> > +statusBarParser :: Parser ()
> > +statusBarParser =
> > + skipSpaces
> > + *> A.char '['
> > + *> A.skipWhile (== '=')
> > + *> A.skipWhile (== '>')
> > + *> A.skipWhile (== '.')
> > + *> A.char ']'
> > + *> pure ()
> > +
> > +-- | The parser for recognizing data size units (only the ones actually
> found in
> > +-- DRBD files are implemented).
> > +sizeUnitParser :: Parser SizeUnit
> > +sizeUnitParser =
> > + kilobyte
> > + <|> megabyte
> > + where kilobyte = A.string "K" *> pure KiloByte
> > + megabyte = A.string "M" *> pure MegaByte
> > +
> > +-- | The parser for recognizing time (hh:mm:ss).
> > +timeParser :: Parser Time
> > +timeParser = Time <$> h <*> m <*> s
> > + where h = A.decimal
> > + m = A.char ':' *> A.decimal
> > + s = A.char ':' *> A.decimal
> > +
> > +-- | The parser for recognizing time units (only the ones actually
> found in DRBD
> > +-- files are implemented).
> > +timeUnitParser :: Parser TimeUnit
> > +timeUnitParser = second
> > + where second = A.string "sec" *> pure Second
> > +
> > +-- | Haskell does not recognises ',' as the separator every 3 digits
> > +-- but DRBD uses it, so we need an ah-hoc parser.
> > +commaIntParser :: Parser Int
> > +commaIntParser = do
> > + first <- A.decimal
> > + allDigits <- commaIntHelper first
> > + pure allDigits
> > +
> > +-- | Helper (triplet parser) for the commaIntParser
> > +commaIntHelper :: Int -> Parser Int
> > +commaIntHelper acc = nextTriplet <|> end
> > + where nextTriplet = do
> > + _ <- A.char ','
> > + triplet <- AC.count 3 A.digit
> > + commaIntHelper $ acc * 1000 + (read triplet :: Int)
> > + end = pure acc
> > +
> > +-- | Parser for the additional information provided by DRBD <= 8.0.
> > +additionalInfoParser::Parser AdditionalInfo
> > +additionalInfoParser = AdditionalInfo
> > + <$> skipSpacesAndString "used:" A.decimal
> > + <*> (A.char '/' *> A.decimal)
> > + <*> skipSpacesAndString "hits:" A.decimal
> > + <*> skipSpacesAndString "misses:" A.decimal
> > + <*> skipSpacesAndString "starving:" A.decimal
> > + <*> skipSpacesAndString "dirty:" A.decimal
> > + <*> skipSpacesAndString "changed:" A.decimal
> > + <* A.endOfLine
> > diff --git a/htools/Ganeti/Block/Drbd/Types.hs
> b/htools/Ganeti/Block/Drbd/Types.hs
> > new file mode 100644
> > index 0000000..20b57f1
> > --- /dev/null
> > +++ b/htools/Ganeti/Block/Drbd/Types.hs
> > @@ -0,0 +1,191 @@
> > +{-| DRBD Data Types
> > +
> > +This module holds the definition of the data types describing the
> status of
> > +DRBD.
> > +
> > +-}
> > +{-
> > +
> > +Copyright (C) 2012 Google Inc.
> > +
> > +This program is free software; you can redistribute it and/or modify
> > +it under the terms of the GNU General Public License as published by
> > +the Free Software Foundation; either version 2 of the License, or
> > +(at your option) any later version.
> > +
> > +This program is distributed in the hope that it will be useful, but
> > +WITHOUT ANY WARRANTY; without even the implied warranty of
> > +MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> > +General Public License for more details.
> > +
> > +You should have received a copy of the GNU General Public License
> > +along with this program; if not, write to the Free Software
> > +Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
> > +02110-1301, USA.
> > +
> > +-}
> > +module Ganeti.Block.Drbd.Types
> > + ( DRBDStatus(..)
> > + , VersionInfo(..)
> > + , DeviceInfo(..)
> > + , ConnectionState(..)
> > + , LocalRemote(..)
> > + , Role(..)
> > + , DiskState(..)
> > + , PerformanceIndicators(..)
> > + , SyncStatus(..)
> > + , SizeUnit(..)
> > + , Time(..)
> > + , TimeUnit(..)
> > + , AdditionalInfo(..)
> > + ) where
> > +
> > +--TODO: consider turning deviceInfos into an IntMap
> > +-- | Data type contaning all the data about the status of DRBD.
> > +data DRBDStatus =
> > + DRBDStatus
> > + { versionInfo :: VersionInfo -- ^ Version information about DRBD
> > + , deviceInfos :: [DeviceInfo] -- ^ Per-minor information
> > + } deriving (Eq, Show)
>
> And the field comments are usually aligned…
>
> > +-- | Data type describing the DRBD version.
> > +data VersionInfo =
> > + VersionInfo
> > + { version :: Maybe String -- ^ DRBD driver version
> > + , api :: Maybe String -- ^ The api version
> > + , proto :: Maybe String -- ^ The protocol version
> > + , srcversion :: Maybe String -- ^ The version of the source files
> > + , gitHash :: Maybe String -- ^ Git hash of the source files
> > + , buildBy :: Maybe String -- ^ Who built the binary (and, optionally,
> when)
> > + } deriving (Eq, Show)
>
> And the fields definitely so.
>
> I'd recommend checking the version I will push against the version you
> have, to see the diffs I applied.
>
Ok, I will.
Thanks,
Michele
