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
HS_LIB_SRCS = \
+ htools/Ganeti/Block/DRBDDataTypes.hs \
+ htools/Ganeti/Block/DRBDParser.hs \
htools/Ganeti/BasicTypes.hs \
htools/Ganeti/Common.hs \
htools/Ganeti/Compat.hs \
@@ -1608,6 +1612,7 @@ hs-apidoc: $(HS_BUILT_SRCS)
rm -rf $(APIDOC_HS_DIR)/*
@mkdir_p@ $(APIDOC_HS_DIR)/Ganeti/HTools/Backend
@mkdir_p@ $(APIDOC_HS_DIR)/Ganeti/HTools/Program
+ @mkdir_p@ $(APIDOC_HS_DIR)/Ganeti/Block
@mkdir_p@ $(APIDOC_HS_DIR)/Ganeti/Confd
@mkdir_p@ $(APIDOC_HS_DIR)/Ganeti/Query
$(HSCOLOUR) -print-css > $(APIDOC_HS_DIR)/Ganeti/hscolour.css
diff --git a/htools/Ganeti/Block/DRBDDataTypes.hs b/htools/Ganeti/Block/DRBDDataTypes.hs
new file mode 100644
index 0000000..19b7ecf
--- /dev/null
+++ b/htools/Ganeti/Block/DRBDDataTypes.hs
@@ -0,0 +1,165 @@
+{-| 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.DRBDDataTypes (DRBDStatus(..),
+ VersionInfo(..),
+ DeviceInfo(..),
+ ConnectionState(..),
+ LocalRemote(..),
+ Role(..),
+ DiskState(..),
+ PerformanceIndicators(..),
+ SyncStatus(..),
+ SizeUnit(..),
+ Time(..),
+ TimeUnit(..),
+ AdditionalInfo(..)) where
+
+-- | Data type contaning all the data about the status of DRBD
+data DRBDStatus = DRBDStatus { versionInfo::VersionInfo,
+ deviceInfo::[DeviceInfo] } deriving (Eq, Show)
+
+-- | Data type describing the DRBD version
+data VersionInfo = VersionInfo { version::Maybe String,
+ api::Maybe String,
+ proto::Maybe String,
+ srcversion::Maybe String,
+ gitHash::Maybe String,
+ buildBy::Maybe String } deriving (Eq, Show)
+
+-- | Data type describing a device
+data DeviceInfo = UnconfiguredDevice Int |
+ DeviceInfo { deviceNumber::Int,
+ connectionState::ConnectionState,
+ resourceRoles::LocalRemote Role,
+ diskStates::LocalRemote DiskState,
+ replicationProtocol::Char,
+ ioFlags::String,
+ performanceIndicators::PerformanceIndicators,
+ syncStatus::Maybe SyncStatus,
+ resync::Maybe AdditionalInfo,
+ actLog::Maybe AdditionalInfo
+ } deriving (Eq, Show)
+
+-- | Data type describing the state of the connection
+data ConnectionState = StandAlone
+ | Disconnecting
+ | Unconnected
+ | Timeout
+ | BrokenPipe
+ | NetworkFailure
+ | ProtocolError
+ | TearDown
+ | WFConnection
+ | WFReportParams
+ | Connected
+ | StartingSyncS
+ | StartingSyncT
+ | WFBitMapS
+ | WFBitMapT
+ | WFSyncUUID
+ | SyncSource
+ | SyncTarget
+ | PausedSyncS
+ | PausedSyncT
+ | VerifyS
+ | VerifyT
+ | Unconfigured deriving (Show, Eq)
+
+-- | Algebraic data type describing something that has a local and a remote
+-- value
+data LocalRemote a = LocalRemote { local::a,
+ remote::a
+ } deriving (Eq, Show)
+
+-- | Data type describing
+data Role = Primary
+ | Secondary
+ | Unknown deriving (Eq, Show)
+
+-- | Data type describing disk states
+data DiskState = Diskless
+ | Attaching
+ | Failed
+ | Negotiating
+ | Inconsistent
+ | Outdated
+ | DUnknown
+ | Consistent
+ | UpToDate deriving (Eq, Show)
+
+-- | Data type containing data about performance indicators
+data PerformanceIndicators = PerformanceIndicators {
+ networkSend::Int,
+ networkReceive::Int,
+ diskWrite::Int,
+ diskRead::Int,
+ activityLog::Int,
+ bitMap::Int,
+ localCount::Int,
+ pending::Int,
+ unacknowledged::Int,
+ applicationPending::Int,
+ epochs::Maybe Int,
+ writeOrder::Maybe Char,
+ outOfSync::Maybe Int } deriving (Eq, Show)
+
+-- | Data type containing data about the synchronization status of a device
+data SyncStatus = SyncStatus {
+ percentage::Double,
+ partialSyncSize::Int,
+ totalSyncSize::Int,
+ syncUnit::SizeUnit,
+ timeToFinish::Time,
+ speed::Double,
+ want::Maybe Double,
+ speedSizeUnit::SizeUnit,
+ speedTimeUnit::TimeUnit
+ } deriving (Eq, Show)
+
+-- | Data type describing a size unit for memory
+data SizeUnit = KiloByte | MegaByte deriving (Eq, Show)
+
+-- | Data type describing a time (hh:mm:ss)
+data Time = Time {
+ hour::Integer,
+ min::Integer,
+ sec::Integer
+ } deriving (Eq, Show)
+
+-- | Data type describing a time unit
+data TimeUnit = Second deriving (Eq, Show)
+
+-- | Additional device-specific information produced by drbd <= 8.0
+data AdditionalInfo = AdditionalInfo {
+ partialUsed::Int,
+ totalUsed::Int,
+ hits::Int,
+ misses::Int,
+ starving::Int,
+ dirty::Int,
+ changed::Int
+ } deriving (Eq, Show)
diff --git a/htools/Ganeti/Block/DRBDParser.hs b/htools/Ganeti/Block/DRBDParser.hs
new file mode 100644
index 0000000..9ed805d
--- /dev/null
+++ b/htools/Ganeti/Block/DRBDParser.hs
@@ -0,0 +1,331 @@
+{-# LANGUAGE OverloadedStrings #-}
+{-| DRBD proc file parser
+
+This module holds the definition of the parser that extracts status information
+from the DRBD proc file
+
+-}
+{-
+
+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.DRBDParser (drbdStatusParser) where
+
+import Control.Applicative ((<*>), (*>), (<*), (<$>), (<|>), pure, some)
+import qualified
This patch set adds the DRBD parser to the ganeti tree.
The parser is, up to now, just used by its unit tests module.
More tests to come soon.
Also, a few utility functions for opening test data files from
Haskell unit tests have been added.
Michele Tartara (4):
Parser for DRBD proc file added
Improved TestHelper module docstring
Utility functions for loading data from test files added
DRBDParser unit test added
diff --git a/htest/Test/Ganeti/TestCommon.hs b/htest/Test/Ganeti/TestCommon.hs
index fdfa7f1..8e8c56b 100644
--- a/htest/Test/Ganeti/TestCommon.hs
+++ b/htest/Test/Ganeti/TestCommon.hs
@@ -217,3 +217,27 @@ testSerialisation a =
resultProp :: (Show a) => BasicTypes.GenericResult a b -> PropertyM IO b
resultProp (BasicTypes.Bad err) = stop . failTest $ show err
resultProp (BasicTypes.Ok val) = return val
+
+-- | Return the source directory of Ganeti
+getSourceDir :: IO FilePath
+getSourceDir = catchJust (guard . isDoesNotExistError)
+ (getEnv "TOP_SRCDIR")
+ (const (return "."))
+
+-- | Returns the path of a file in the test data directory, given its name
+testDataFilename :: String -> String -> IO FilePath
+testDataFilename datadir name = do
+ src <- getSourceDir
+ return $ src ++ datadir ++ name
+
+-- | Returns the content of the specified python test data file
+readPythonTestData :: String -> IO String
+readPythonTestData filename = do
+ name <- testDataFilename "/test/data/" filename
+ readFile name
+
+-- | Returns the content of the specified haskell test data file
+readTestData :: String -> IO String
+readTestData filename = do
+ name <- testDataFilename "/htest/data/" filename
+ readFile name
-- 1.7.7.3
> +getSourceDir :: IO FilePath
> +getSourceDir = catchJust (guard . isDoesNotExistError)
> + (getEnv "TOP_SRCDIR")
> + (const (return "."))
> +
> +-- | Returns the path of a file in the test data directory, given its name
> +testDataFilename :: String -> String -> IO FilePath
> +testDataFilename datadir name = do
> + src <- getSourceDir
> + return $ src ++ datadir ++ name
> +
> +-- | Returns the content of the specified python test data file
> +readPythonTestData :: String -> IO String
> +readPythonTestData filename = do
> + name <- testDataFilename "/test/data/" filename
> + readFile name
> +
> +-- | Returns the content of the specified haskell test data file
> +readTestData :: String -> IO String
For consistency, should this be readHaskellTestData? Just wondering.
On Fri, Nov 16, 2012 at 11:01 AM, Iustin Pop <ius...@google.com> wrote:
> On Fri, Nov 16, 2012 at 09:53:38AM +0100, Michele Tartara wrote:
> > They mimic their python counterparts.
> > diff --git a/htest/Test/Ganeti/TestCommon.hs
> b/htest/Test/Ganeti/TestCommon.hs
> > index fdfa7f1..8e8c56b 100644
> > --- a/htest/Test/Ganeti/TestCommon.hs
> > +++ b/htest/Test/Ganeti/TestCommon.hs
> > @@ -217,3 +217,27 @@ testSerialisation a =
> > resultProp :: (Show a) => BasicTypes.GenericResult a b -> PropertyM IO b
> > resultProp (BasicTypes.Bad err) = stop . failTest $ show err
> > resultProp (BasicTypes.Ok val) = return val
> > +
> > +-- | Return the source directory of Ganeti
> '.' at end of docstrings; same for the other functions.
> Ok, I'll fix it.
> > +getSourceDir :: IO FilePath
> > +getSourceDir = catchJust (guard . isDoesNotExistError)
> > + (getEnv "TOP_SRCDIR")
> > + (const (return "."))
> > +
> > +-- | Returns the path of a file in the test data directory, given its
> name
> > +testDataFilename :: String -> String -> IO FilePath
> > +testDataFilename datadir name = do
> > + src <- getSourceDir
> > + return $ src ++ datadir ++ name
> > +
> > +-- | Returns the content of the specified python test data file
> > +readPythonTestData :: String -> IO String
> > +readPythonTestData filename = do
> > + name <- testDataFilename "/test/data/" filename
> > + readFile name
> > +
> > +-- | Returns the content of the specified haskell test data file
> > +readTestData :: String -> IO String
> For consistency, should this be readHaskellTestData? Just wondering.
I thought about that, but in the end I preferred to go for a "default
implementation" for the test data specific for the current language and a
"named" implementation for the external language (python).
Also because in my opinion having a single test data directory would make
more sense (data are not language specific) and if they ever end up being
joined, a language-agnostic function to access the data would make more
sense.
> > > diff --git a/htest/Test/Ganeti/TestCommon.hs
> > b/htest/Test/Ganeti/TestCommon.hs
> > > index fdfa7f1..8e8c56b 100644
> > > --- a/htest/Test/Ganeti/TestCommon.hs
> > > +++ b/htest/Test/Ganeti/TestCommon.hs
> > > @@ -217,3 +217,27 @@ testSerialisation a =
> > > resultProp :: (Show a) => BasicTypes.GenericResult a b -> PropertyM IO b
> > > resultProp (BasicTypes.Bad err) = stop . failTest $ show err
> > > resultProp (BasicTypes.Ok val) = return val
> > > +
> > > +-- | Return the source directory of Ganeti
> > '.' at end of docstrings; same for the other functions.
> > Ok, I'll fix it.
> > > +getSourceDir :: IO FilePath
> > > +getSourceDir = catchJust (guard . isDoesNotExistError)
> > > + (getEnv "TOP_SRCDIR")
> > > + (const (return "."))
> > > +
> > > +-- | Returns the path of a file in the test data directory, given its
> > name
> > > +testDataFilename :: String -> String -> IO FilePath
> > > +testDataFilename datadir name = do
> > > + src <- getSourceDir
> > > + return $ src ++ datadir ++ name
> > > +
> > > +-- | Returns the content of the specified python test data file
> > > +readPythonTestData :: String -> IO String
> > > +readPythonTestData filename = do
> > > + name <- testDataFilename "/test/data/" filename
> > > + readFile name
> > > +
> > > +-- | Returns the content of the specified haskell test data file
> > > +readTestData :: String -> IO String
> > For consistency, should this be readHaskellTestData? Just wondering.
> I thought about that, but in the end I preferred to go for a "default
> implementation" for the test data specific for the current language and a
> "named" implementation for the external language (python).
Ack, makes sense.
> Also because in my opinion having a single test data directory would make
> more sense (data are not language specific) and if they ever end up being
> joined, a language-agnostic function to access the data would make more
> sense.
Hmm. We actually had them (test data) together, but then I split them
for clarity.
Maybe we want to change the tree as follows?
\-pytest
\-hstest
\-test-data
So that the test files are separated, but test data files are common?
On Fri, Nov 16, 2012 at 1:15 PM, Iustin Pop <ius...@google.com> wrote:
> On Fri, Nov 16, 2012 at 01:05:53PM +0100, Michele Tartara wrote:
> > On Fri, Nov 16, 2012 at 11:01 AM, Iustin Pop <ius...@google.com> wrote:
> > > On Fri, Nov 16, 2012 at 09:53:38AM +0100, Michele Tartara wrote:
> > > > They mimic their python counterparts.
> > > > diff --git a/htest/Test/Ganeti/TestCommon.hs
> > > b/htest/Test/Ganeti/TestCommon.hs
> > > > index fdfa7f1..8e8c56b 100644
> > > > --- a/htest/Test/Ganeti/TestCommon.hs
> > > > +++ b/htest/Test/Ganeti/TestCommon.hs
> > > > @@ -217,3 +217,27 @@ testSerialisation a =
> > > > resultProp :: (Show a) => BasicTypes.GenericResult a b -> PropertyM
> IO b
> > > > resultProp (BasicTypes.Bad err) = stop . failTest $ show err
> > > > resultProp (BasicTypes.Ok val) = return val
> > > > +
> > > > +-- | Return the source directory of Ganeti
> > > '.' at end of docstrings; same for the other functions.
> > > Ok, I'll fix it.
> > > > +getSourceDir :: IO FilePath
> > > > +getSourceDir = catchJust (guard . isDoesNotExistError)
> > > > + (getEnv "TOP_SRCDIR")
> > > > + (const (return "."))
> > > > +
> > > > +-- | Returns the path of a file in the test data directory, given
> its
> > > name
> > > > +testDataFilename :: String -> String -> IO FilePath
> > > > +testDataFilename datadir name = do
> > > > + src <- getSourceDir
> > > > + return $ src ++ datadir ++ name
> > > > +
> > > > +-- | Returns the content of the specified python test data file
> > > > +readPythonTestData :: String -> IO String
> > > > +readPythonTestData filename = do
> > > > + name <- testDataFilename "/test/data/" filename
> > > > + readFile name
> > > > +
> > > > +-- | Returns the content of the specified haskell test data file
> > > > +readTestData :: String -> IO String
> > > For consistency, should this be readHaskellTestData? Just wondering.
> > I thought about that, but in the end I preferred to go for a "default
> > implementation" for the test data specific for the current language and a
> > "named" implementation for the external language (python).
> Ack, makes sense.
Ok, I'm resending the patch with the docstring's full stops added, and
without changing the function names.
> > Also because in my opinion having a single test data directory would make
> > more sense (data are not language specific) and if they ever end up being
> > joined, a language-agnostic function to access the data would make more
> > sense.
> Hmm. We actually had them (test data) together, but then I split them
> for clarity.
> Maybe we want to change the tree as follows?
> \-pytest
> \-hstest
> \-test-data
> So that the test files are separated, but test data files are common?
It sounds like the best approach to me.
But that definitely goes in a different patch! :-p
diff --git a/htest/Test/Ganeti/TestCommon.hs b/htest/Test/Ganeti/TestCommon.hs
index fdfa7f1..c108079 100644
--- a/htest/Test/Ganeti/TestCommon.hs
+++ b/htest/Test/Ganeti/TestCommon.hs
@@ -217,3 +217,27 @@ testSerialisation a =
resultProp :: (Show a) => BasicTypes.GenericResult a b -> PropertyM IO b
resultProp (BasicTypes.Bad err) = stop . failTest $ show err
resultProp (BasicTypes.Ok val) = return val
+
+-- | Return the source directory of Ganeti.
+getSourceDir :: IO FilePath
+getSourceDir = catchJust (guard . isDoesNotExistError)
+ (getEnv "TOP_SRCDIR")
+ (const (return "."))
+
+-- | Returns the path of a file in the test data directory, given its name.
+testDataFilename :: String -> String -> IO FilePath
+testDataFilename datadir name = do
+ src <- getSourceDir
+ return $ src ++ datadir ++ name
+
+-- | Returns the content of the specified python test data file.
+readPythonTestData :: String -> IO String
+readPythonTestData filename = do
+ name <- testDataFilename "/test/data/" filename
+ readFile name
+
+-- | Returns the content of the specified haskell test data file.
+readTestData :: String -> IO String
+readTestData filename = do
+ name <- testDataFilename "/htest/data/" filename
+ readFile name
-- 1.7.7.3
> > > > '.' at end of docstrings; same for the other functions.
> > > > Ok, I'll fix it.
> > > > > +getSourceDir :: IO FilePath
> > > > > +getSourceDir = catchJust (guard . isDoesNotExistError)
> > > > > + (getEnv "TOP_SRCDIR")
> > > > > + (const (return "."))
> > > > > +
> > > > > +-- | Returns the path of a file in the test data directory, given
> > its
> > > > name
> > > > > +testDataFilename :: String -> String -> IO FilePath
> > > > > +testDataFilename datadir name = do
> > > > > + src <- getSourceDir
> > > > > + return $ src ++ datadir ++ name
> > > > > +
> > > > > +-- | Returns the content of the specified python test data file
> > > > > +readPythonTestData :: String -> IO String
> > > > > +readPythonTestData filename = do
> > > > > + name <- testDataFilename "/test/data/" filename
> > > > > + readFile name
> > > > > +
> > > > > +-- | Returns the content of the specified haskell test data file
> > > > > +readTestData :: String -> IO String
> > > > For consistency, should this be readHaskellTestData? Just wondering.
> > > I thought about that, but in the end I preferred to go for a "default
> > > implementation" for the test data specific for the current language and a
> > > "named" implementation for the external language (python).
> > Ack, makes sense.
> Ok, I'm resending the patch with the docstring's full stops added, and
> without changing the function names.
> > > Also because in my opinion having a single test data directory would make
> > > more sense (data are not language specific) and if they ever end up being
> > > joined, a language-agnostic function to access the data would make more
> > > sense.
> > Hmm. We actually had them (test data) together, but then I split them
> > for clarity.
> > Maybe we want to change the tree as follows?
> > \-pytest
> > \-hstest
> > \-test-data
> > So that the test files are separated, but test data files are common?
> It sounds like the best approach to me.
> But that definitely goes in a different patch! :-p
Absolutely, was just discussing here as it came up.
On Fri, Nov 16, 2012 at 09:53:36AM +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
> +++ b/htools/Ganeti/Block/DRBDDataTypes.hs
> @@ -0,0 +1,165 @@
> +{-| 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.DRBDDataTypes (DRBDStatus(..),
> + VersionInfo(..),
> + DeviceInfo(..),
> + ConnectionState(..),
> + LocalRemote(..),
> + Role(..),
> + DiskState(..),
> + PerformanceIndicators(..),
> + SyncStatus(..),
> + SizeUnit(..),
> + Time(..),
> + TimeUnit(..),
> + AdditionalInfo(..)) where
Please use, here and in all multi-line declarations below, Haskell-style
indendation - check the other source files. This should be rather:
module Test.Ganeti.TestHelper
( testSuite
, genArbitrary
, …
) where
> +-- | Data type contaning all the data about the status of DRBD
> +data DRBDStatus = DRBDStatus { versionInfo::VersionInfo,
> + deviceInfo::[DeviceInfo] } deriving (Eq, Show)
White space around '::'. Deriving should be on its own line. Docstrings
for the actual member types.
> diff --git a/htools/Ganeti/Block/DRBDParser.hs b/htools/Ganeti/Block/DRBDParser.hs
> new file mode 100644
> index 0000000..9ed805d
> --- /dev/null
> +++ b/htools/Ganeti/Block/DRBDParser.hs
> @@ -0,0 +1,331 @@
> +{-# LANGUAGE OverloadedStrings #-}
> +{-| DRBD proc file parser
> +
> +This module holds the definition of the parser that extracts status information
> +from the DRBD proc file
> +
> +-}
> +{-
> +
> +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.DRBDParser (drbdStatusParser) where
> +
> +import Control.Applicative ((<*>), (*>), (<*), (<$>), (<|>), pure, some)
> +import qualified Data.Attoparsec.Text as A
> +import qualified Data.Attoparsec.Combinator as AC
> +import Data.Attoparsec.Text (Parser)
> +import Data.Text (Text, unpack)
> +
> +import
> diff --git a/Makefile.am b/Makefile.am
> index 13119f0..46717c3 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -66,6 +66,7 @@ HTOOLS_DIRS = \
> htest \
> htest/Test \
> htest/Test/Ganeti \
> + htest/Test/Ganeti/Block \
> htest/Test/Ganeti/Confd \
> htest/Test/Ganeti/HTools \
> htest/Test/Ganeti/HTools/Backend \
> @@ -487,6 +488,7 @@ HS_TEST_SRCS = \
> htest/Test/Ganeti/Common.hs \
> htest/Test/Ganeti/Confd/Utils.hs \
> htest/Test/Ganeti/Daemon.hs \
> + htest/Test/Ganeti/Block/DRBDParser.hs \
> htest/Test/Ganeti/Errors.hs \
> htest/Test/Ganeti/HTools/Backend/Simu.hs \
> htest/Test/Ganeti/HTools/Backend/Text.hs \
> diff --git a/htest/Test/Ganeti/Block/DRBDParser.hs b/htest/Test/Ganeti/Block/DRBDParser.hs
> new file mode 100644
> index 0000000..7f2780d
> --- /dev/null
> +++ b/htest/Test/Ganeti/Block/DRBDParser.hs
> @@ -0,0 +1,122 @@
> +{-# LANGUAGE TemplateHaskell #-}
> +
> +{-| Unittests for Attoparsec support for unicode -}
> +
> +{-
> +
> +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 Test.Ganeti.Block.DRBDParser (testBlock_DRBDParser) where
> +
> +import Test.HUnit
> +
> +import Test.Ganeti.TestHelper
> +import Test.Ganeti.TestCommon (readPythonTestData)
> +
> +import qualified Data.Attoparsec.Text as A
> +import Data.Text (pack)
> +
> +import Ganeti.Block.DRBDParser (drbdStatusParser)
> +import Ganeti.Block.DRBDDataTypes
> +
> +case_drbd8 :: Assertion
> +case_drbd8 = do
> + fileContent <- readPythonTestData "proc_drbd8.txt"
Wrong indentation here - 2 spaces, not 4. Possible in the other patches
as well.
Also, I would really prefer if we test all existing test files; we have
multiple ones especially to handle variations in /proc/drbd format.
On Fri, Nov 16, 2012 at 1:43 PM, Iustin Pop <ius...@google.com> wrote:
> On Fri, Nov 16, 2012 at 09:53:36AM +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
> > +++ b/htools/Ganeti/Block/DRBDDataTypes.hs
> > @@ -0,0 +1,165 @@
> > +{-| 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.DRBDDataTypes (DRBDStatus(..),
> > + VersionInfo(..),
> > + DeviceInfo(..),
> > + ConnectionState(..),
> > + LocalRemote(..),
> > + Role(..),
> > + DiskState(..),
> > + PerformanceIndicators(..),
> > + SyncStatus(..),
> > + SizeUnit(..),
> > + Time(..),
> > + TimeUnit(..),
> > + AdditionalInfo(..)) where
> Please use, here and in all multi-line declarations below, Haskell-style
> indendation - check the other source files. This should be rather:
Ok, I'll find the proper style guide (if it exists) or I will just look at
the rest of the code.
> > +-- | Data type contaning all the data about the status of DRBD
> > +data DRBDStatus = DRBDStatus { versionInfo::VersionInfo,
> > + deviceInfo::[DeviceInfo] } deriving (Eq,
> Show)
> White space around '::'. Deriving should be on its own line. Docstrings
> for the actual member types.
Ok for the spaces and and for "deriving" on its own line.
But what do you mean exactly with "actual member types"? One docstring for
every constructor?
> > diff --git a/htools/Ganeti/Block/DRBDParser.hs
> b/htools/Ganeti/Block/DRBDParser.hs
> > new file mode 100644
> > index 0000000..9ed805d
> > --- /dev/null
> > +++ b/htools/Ganeti/Block/DRBDParser.hs
> > @@ -0,0 +1,331 @@
> > +{-# LANGUAGE OverloadedStrings #-}
> > +{-| DRBD proc file parser
> > +
> > +This module holds the definition of the parser that extracts status
> information
> > +from the DRBD proc file
> > +
> > +-}
> > +{-
> > +
> > +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
On Fri, Nov 16, 2012 at 1:45 PM, Iustin Pop <ius...@google.com> wrote:
> On Fri, Nov 16, 2012 at 09:53:39AM +0100, Michele Tartara wrote:
> > Signed-off-by: Michele Tartara <mtart...@google.com>
> > ---
> > Makefile.am | 2 +
> > htest/Test/Ganeti/Block/DRBDParser.hs | 122
> +++++++++++++++++++++++++++++++++
> > htest/test.hs | 2 +
> > 3 files changed, 126 insertions(+), 0 deletions(-)
> > create mode 100644 htest/Test/Ganeti/Block/DRBDParser.hs
> > diff --git a/Makefile.am b/Makefile.am
> > index 13119f0..46717c3 100644
> > --- a/Makefile.am
> > +++ b/Makefile.am
> > @@ -66,6 +66,7 @@ HTOOLS_DIRS = \
> > htest \
> > htest/Test \
> > htest/Test/Ganeti \
> > + htest/Test/Ganeti/Block \
> > htest/Test/Ganeti/Confd \
> > htest/Test/Ganeti/HTools \
> > htest/Test/Ganeti/HTools/Backend \
> > @@ -487,6 +488,7 @@ HS_TEST_SRCS = \
> > htest/Test/Ganeti/Common.hs \
> > htest/Test/Ganeti/Confd/Utils.hs \
> > htest/Test/Ganeti/Daemon.hs \
> > + htest/Test/Ganeti/Block/DRBDParser.hs \
> > htest/Test/Ganeti/Errors.hs \
> > htest/Test/Ganeti/HTools/Backend/Simu.hs \
> > htest/Test/Ganeti/HTools/Backend/Text.hs \
> > diff --git a/htest/Test/Ganeti/Block/DRBDParser.hs
> b/htest/Test/Ganeti/Block/DRBDParser.hs
> > new file mode 100644
> > index 0000000..7f2780d
> > --- /dev/null
> > +++ b/htest/Test/Ganeti/Block/DRBDParser.hs
> > @@ -0,0 +1,122 @@
> > +{-# LANGUAGE TemplateHaskell #-}
> > +
> > +{-| Unittests for Attoparsec support for unicode -}
> > +
> > +{-
> > +
> > +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 Test.Ganeti.Block.DRBDParser (testBlock_DRBDParser) where
> > +
> > +import Test.HUnit
> > +
> > +import Test.Ganeti.TestHelper
> > +import Test.Ganeti.TestCommon (readPythonTestData)
> > +
> > +import qualified Data.Attoparsec.Text as A
> > +import Data.Text (pack)
> > +
> > +import Ganeti.Block.DRBDParser (drbdStatusParser)
> > +import Ganeti.Block.DRBDDataTypes
> > +
> > +case_drbd8 :: Assertion
> > +case_drbd8 = do
> > + fileContent <- readPythonTestData "proc_drbd8.txt"
> Wrong indentation here - 2 spaces, not 4. Possible in the other patches
> as well.
Ok, I'll fix it.
> Also, I would really prefer if we test all existing test files; we have
> multiple ones especially to handle variations in /proc/drbd format.
This has always been my plan as well :-)
I already wrote some more of them, and I'm writing the others. That's what
I meant with "More tests to come" in the cover letter.
I just wanted to submit something as soon as I could, in order to get
feedback on the coding style, so that I can directly write the rest of the
tests (and of the code, in general) with the correct style, without wasting
time reformatting them afterwards.
On Fri, Nov 16, 2012 at 02:42:19PM +0100, Michele Tartara wrote:
> On Fri, Nov 16, 2012 at 1:43 PM, Iustin Pop <ius...@google.com> wrote:
> > On Fri, Nov 16, 2012 at 09:53:36AM +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
> > > +++ b/htools/Ganeti/Block/DRBDDataTypes.hs
> > > @@ -0,0 +1,165 @@
> > > +{-| 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.DRBDDataTypes (DRBDStatus(..),
> > > + VersionInfo(..),
> > > + DeviceInfo(..),
> > > + ConnectionState(..),
> > > + LocalRemote(..),
> > > + Role(..),
> > > + DiskState(..),
> > > + PerformanceIndicators(..),
> > > + SyncStatus(..),
> > > + SizeUnit(..),
> > > + Time(..),
> > > + TimeUnit(..),
> > > + AdditionalInfo(..)) where
> > Please use, here and in all multi-line declarations below, Haskell-style
> > indendation - check the other source files. This should be rather:
> > > +-- | Data type contaning all the data about the status of DRBD
> > > +data DRBDStatus = DRBDStatus { versionInfo::VersionInfo,
> > > + deviceInfo::[DeviceInfo] } deriving (Eq,
> > Show)
> > White space around '::'. Deriving should be on its own line. Docstrings
> > for the actual member types.
> Ok for the spaces and and for "deriving" on its own line.
> But what do you mean exactly with "actual member types"? One docstring for
> every constructor?
Yes, for both constructor and record names. Since you have only one
constructor here, we don't need to document it, but we need to document
the record fields:
data DRBDStatus = DRBDStatus
{ versionInfo :: VersionInfo -- ^ The driver version
, deviceInfo :: [DeviceInfo] -- ^ Per-minor information
} deriving (Show, Read, Eq)
Speaking of which. Why is deviceInfo a list? Is the minor index itself
present in DeviceInfo?
> > > diff --git a/Makefile.am b/Makefile.am
> > > index 13119f0..46717c3 100644
> > > --- a/Makefile.am
> > > +++ b/Makefile.am
> > > @@ -66,6 +66,7 @@ HTOOLS_DIRS = \
> > > htest \
> > > htest/Test \
> > > htest/Test/Ganeti \
> > > + htest/Test/Ganeti/Block \
> > > htest/Test/Ganeti/Confd \
> > > htest/Test/Ganeti/HTools \
> > > htest/Test/Ganeti/HTools/Backend \
> > > @@ -487,6 +488,7 @@ HS_TEST_SRCS = \
> > > htest/Test/Ganeti/Common.hs \
> > > htest/Test/Ganeti/Confd/Utils.hs \
> > > htest/Test/Ganeti/Daemon.hs \
> > > + htest/Test/Ganeti/Block/DRBDParser.hs \
> > > htest/Test/Ganeti/Errors.hs \
> > > htest/Test/Ganeti/HTools/Backend/Simu.hs \
> > > htest/Test/Ganeti/HTools/Backend/Text.hs \
> > > diff --git a/htest/Test/Ganeti/Block/DRBDParser.hs
> > b/htest/Test/Ganeti/Block/DRBDParser.hs
> > > new file mode 100644
> > > index 0000000..7f2780d
> > > --- /dev/null
> > > +++ b/htest/Test/Ganeti/Block/DRBDParser.hs
> > > @@ -0,0 +1,122 @@
> > > +{-# LANGUAGE TemplateHaskell #-}
> > > +
> > > +{-| Unittests for Attoparsec support for unicode -}
> > > +
> > > +{-
> > > +
> > > +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 Test.Ganeti.Block.DRBDParser (testBlock_DRBDParser) where
> > > +
> > > +import Test.HUnit
> > > +
> > > +import Test.Ganeti.TestHelper
> > > +import Test.Ganeti.TestCommon (readPythonTestData)
> > > +
> > > +import qualified Data.Attoparsec.Text as A
> > > +import Data.Text (pack)
> > > +
> > > +import Ganeti.Block.DRBDParser (drbdStatusParser)
> > > +import Ganeti.Block.DRBDDataTypes
> > > +
> > > +case_drbd8 :: Assertion
> > > +case_drbd8 = do
> > > + fileContent <- readPythonTestData "proc_drbd8.txt"
> > Wrong indentation here - 2 spaces, not 4. Possible in the other patches
> > as well.
> Ok, I'll fix it.
> > Also, I would really prefer if we test all existing test files; we have
> > multiple ones especially to handle variations in /proc/drbd format.
> This has always been my plan as well :-)
> I already wrote some more of them, and I'm writing the others. That's what
> I meant with "More tests to come" in the cover letter.
> I just wanted to submit something as soon as I could, in order to get
> feedback on the coding style, so that I can directly write the rest of the
> tests (and of the code, in general) with the correct style, without wasting
> time reformatting them afterwards.
Oh sorry, I didn't see that (more to come later). Sounds good, thanks!
On Fri, Nov 16, 2012 at 2:58 PM, Iustin Pop <ius...@google.com> wrote:
> On Fri, Nov 16, 2012 at 02:42:19PM +0100, Michele Tartara wrote:
> > On Fri, Nov 16, 2012 at 1:43 PM, Iustin Pop <ius...@google.com> wrote:
> > > On Fri, Nov 16, 2012 at 09:53:36AM +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
> > > > +++ b/htools/Ganeti/Block/DRBDDataTypes.hs
> > > > @@ -0,0 +1,165 @@
> > > > +{-| 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.DRBDDataTypes (DRBDStatus(..),
> > > > + VersionInfo(..),
> > > > + DeviceInfo(..),
> > > > + ConnectionState(..),
> > > > + LocalRemote(..),
> > > > + Role(..),
> > > > + DiskState(..),
> > > > + PerformanceIndicators(..),
> > > > + SyncStatus(..),
> > > > + SizeUnit(..),
> > > > + Time(..),
> > > > + TimeUnit(..),
> > > > + AdditionalInfo(..)) where
> > > Please use, here and in all multi-line declarations below,
> Haskell-style
> > > indendation - check the other source files. This should be rather:
> > > > +-- | Data type contaning all the data about the status of DRBD
> > > > +data DRBDStatus = DRBDStatus { versionInfo::VersionInfo,
> > > > + deviceInfo::[DeviceInfo] } deriving
> (Eq,
> > > Show)
> > > White space around '::'. Deriving should be on its own line. Docstrings
> > > for the actual member types.
> > Ok for the spaces and and for "deriving" on its own line.
> > But what do you mean exactly with "actual member types"? One docstring
> for
> > every constructor?
> Yes, for both constructor and record names. Since you have only one
> constructor here, we don't need to document it, but we need to document
> the record fields:
> Ok
> data DRBDStatus = DRBDStatus
> { versionInfo :: VersionInfo -- ^ The driver version
> , deviceInfo :: [DeviceInfo] -- ^ Per-minor information
> } deriving (Show, Read, Eq)
> Speaking of which. Why is deviceInfo a list? Is the minor index itself
> present in DeviceInfo?
Yes, the minor index is the deviceNumber field of DeviceInfo (probably I
should change the name to minorNumber).
Each "DeviceInfo" represents a single DRBD device, that's why it's a list:
you can have multiple DeviceInfos.
Why by the way makes me think that the deviceInfo field of DRBDStatus
should be renamed to deviceInfos.
On Fri, Nov 16, 2012 at 03:26:26PM +0100, Michele Tartara wrote:
> On Fri, Nov 16, 2012 at 2:58 PM, Iustin Pop <ius...@google.com> wrote:
> > On Fri, Nov 16, 2012 at 02:42:19PM +0100, Michele Tartara wrote:
> > > On Fri, Nov 16, 2012 at 1:43 PM, Iustin Pop <ius...@google.com> wrote:
> > > > On Fri, Nov 16, 2012 at 09:53:36AM +0100, Michele Tartara wrote:
> > > > > +-- | Data type contaning all the data about the status of DRBD
> > > > > +data DRBDStatus = DRBDStatus { versionInfo::VersionInfo,
> > > > > + deviceInfo::[DeviceInfo] } deriving
> > (Eq,
> > > > Show)
> > > > White space around '::'. Deriving should be on its own line. Docstrings
> > > > for the actual member types.
> > > Ok for the spaces and and for "deriving" on its own line.
> > > But what do you mean exactly with "actual member types"? One docstring
> > for
> > > every constructor?
> > Yes, for both constructor and record names. Since you have only one
> > constructor here, we don't need to document it, but we need to document
> > the record fields:
> > Ok
> > data DRBDStatus = DRBDStatus
> > { versionInfo :: VersionInfo -- ^ The driver version
> > , deviceInfo :: [DeviceInfo] -- ^ Per-minor information
> > } deriving (Show, Read, Eq)
> > Speaking of which. Why is deviceInfo a list? Is the minor index itself
> > present in DeviceInfo?
> Yes, the minor index is the deviceNumber field of DeviceInfo (probably I
> should change the name to minorNumber).
> Each "DeviceInfo" represents a single DRBD device, that's why it's a list:
> you can have multiple DeviceInfos.
> Why by the way makes me think that the deviceInfo field of DRBDStatus
> should be renamed to deviceInfos.
Yes. I also wonder if this shouldn't be a Data.IntMap rather than a
list; well, if we never want to lookup a given device by index, then not
necessarily.
> > > > Also, while the code looks good for now, I'll make the note that, for
> > > > example, this seems quite uniform/regular, so it could be
> > > > simplified/generated via TH.
> > > I'll have a look at it for the future, then. I really like automatic code
> > > generation, so TH looks really interesting to me.
> > > But, writing my first Haskell program, my first attoparsec parser and my
> > > first template haskell program all in one go would have probably been too
> > > much :-p
> > Totally agreed. Just saying that this particular type (not the others)
> > seems very 'regular' (if we can say that).
> All the "leaf" types of the parsing tree that are composed by a fixed set
> of alternatives look like this (ConnectionState, Role, DeviceState, etc...)
> It would actually made a lot of sense (for the future data collectors) for
> me to learn TH to implement them.
OK, not for now, but let me know when you want to start that, and I
could explain how our TH code is structured.
On Fri, Nov 16, 2012 at 3:30 PM, Iustin Pop <ius...@google.com> wrote:
> On Fri, Nov 16, 2012 at 03:26:26PM +0100, Michele Tartara wrote:
> > On Fri, Nov 16, 2012 at 2:58 PM, Iustin Pop <ius...@google.com> wrote:
> > > On Fri, Nov 16, 2012 at 02:42:19PM +0100, Michele Tartara wrote:
> > > > On Fri, Nov 16, 2012 at 1:43 PM, Iustin Pop <ius...@google.com>
> wrote:
> > > > > On Fri, Nov 16, 2012 at 09:53:36AM +0100, Michele Tartara wrote:
> > > > > > +-- | Data type contaning all the data about the status of DRBD
> > > > > > +data DRBDStatus = DRBDStatus { versionInfo::VersionInfo,
> > > > > > + deviceInfo::[DeviceInfo] }
> deriving
> > > (Eq,
> > > > > Show)
> > > > > White space around '::'. Deriving should be on its own line.
> Docstrings
> > > > > for the actual member types.
> > > > Ok for the spaces and and for "deriving" on its own line.
> > > > But what do you mean exactly with "actual member types"? One
> docstring
> > > for
> > > > every constructor?
> > > Yes, for both constructor and record names. Since you have only one
> > > constructor here, we don't need to document it, but we need to document
> > > the record fields:
> > > Ok
> > > data DRBDStatus = DRBDStatus
> > > { versionInfo :: VersionInfo -- ^ The driver version
> > > , deviceInfo :: [DeviceInfo] -- ^ Per-minor
> information
> > > } deriving (Show, Read, Eq)
> > > Speaking of which. Why is deviceInfo a list? Is the minor index itself
> > > present in DeviceInfo?
> > Yes, the minor index is the deviceNumber field of DeviceInfo (probably I
> > should change the name to minorNumber).
> > Each "DeviceInfo" represents a single DRBD device, that's why it's a
> list:
> > you can have multiple DeviceInfos.
> > Why by the way makes me think that the deviceInfo field of DRBDStatus
> > should be renamed to deviceInfos.
> Yes. I also wonder if this shouldn't be a Data.IntMap rather than a
> list; well, if we never want to lookup a given device by index, then not
> necessarily.
It might be a good idea. On the other hand, I don't think the list would be
extremely long anyway, so I guess the speedup of not using a list would be
quite negligible.
I will think about it while I restructure the rest of the code according to
your suggestions.
> > > > > Also, while the code looks good for now, I'll make the note that,
> for
> > > > > example, this seems quite uniform/regular, so it could be
> > > > > simplified/generated via TH.
> > > > I'll have a look at it for the future, then. I really like automatic
> code
> > > > generation, so TH looks really interesting to me.
> > > > But, writing my first Haskell program, my first attoparsec parser
> and my
> > > > first template haskell program all in one go would have probably
> been too
> > > > much :-p
> > > Totally agreed. Just saying that this particular type (not the others)
> > > seems very 'regular' (if we can say that).
> > All the "leaf" types of the parsing tree that are composed by a fixed set
> > of alternatives look like this (ConnectionState, Role, DeviceState,
> etc...)
> > It would actually made a lot of sense (for the future data collectors)
> for
> > me to learn TH to implement them.
> OK, not for now, but let me know when you want to start that, and I
> could explain how our TH code is structured.
On Fri, Nov 16, 2012 at 03:33:40PM +0100, Michele Tartara wrote:
> On Fri, Nov 16, 2012 at 3:30 PM, Iustin Pop <ius...@google.com> wrote:
> > On Fri, Nov 16, 2012 at 03:26:26PM +0100, Michele Tartara wrote:
> > > On Fri, Nov 16, 2012 at 2:58 PM, Iustin Pop <ius...@google.com> wrote:
> > > > On Fri, Nov 16, 2012 at 02:42:19PM +0100, Michele Tartara wrote:
> > > > > On Fri, Nov 16, 2012 at 1:43 PM, Iustin Pop <ius...@google.com>
> > wrote:
> > > > > > On Fri, Nov 16, 2012 at 09:53:36AM +0100, Michele Tartara wrote:
> > > > > > > +-- | Data type contaning all the data about the status of DRBD
> > > > > > > +data DRBDStatus = DRBDStatus { versionInfo::VersionInfo,
> > > > > > > + deviceInfo::[DeviceInfo] }
> > deriving
> > > > (Eq,
> > > > > > Show)
> > > > > > White space around '::'. Deriving should be on its own line.
> > Docstrings
> > > > > > for the actual member types.
> > > > > Ok for the spaces and and for "deriving" on its own line.
> > > > > But what do you mean exactly with "actual member types"? One
> > docstring
> > > > for
> > > > > every constructor?
> > > > Yes, for both constructor and record names. Since you have only one
> > > > constructor here, we don't need to document it, but we need to document
> > > > the record fields:
> > > > Speaking of which. Why is deviceInfo a list? Is the minor index itself
> > > > present in DeviceInfo?
> > > Yes, the minor index is the deviceNumber field of DeviceInfo (probably I
> > > should change the name to minorNumber).
> > > Each "DeviceInfo" represents a single DRBD device, that's why it's a
> > list:
> > > you can have multiple DeviceInfos.
> > > Why by the way makes me think that the deviceInfo field of DRBDStatus
> > > should be renamed to deviceInfos.
> > Yes. I also wonder if this shouldn't be a Data.IntMap rather than a
> > list; well, if we never want to lookup a given device by index, then not
> > necessarily.
> It might be a good idea. On the other hand, I don't think the list would be
> extremely long anyway, so I guess the speedup of not using a list would be
> quite negligible.
> I will think about it while I restructure the rest of the code according to
> your suggestions.
Oh, speedup was not necessarily my goal. But the fact that a Map
represents the structure of the data better (unique minor indices,
etc.). Whereas a list only says there could be "some" devices.
Anyway, please don't rewrite now, just add a TODO. That's minor point.
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
