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
After looking some more at the types, I have some notes. Nothing needs
to change now, just wanted to discuss a bit.
> +-- | Data type describing a device.
> +data DeviceInfo =
> + UnconfiguredDevice Int -- ^ An DRBD minor marked as unconfigured
> + | -- | A configured DRBD minor
> + DeviceInfo
> + { minorNumber :: Int -- ^ The minor index of the device
> + , connectionState :: ConnectionState -- ^ State of the connection
> + , resourceRoles :: LocalRemote Role -- ^ Roles of the resources
> + , diskStates :: LocalRemote DiskState -- ^ Status of the disks
> + , replicationProtocol :: Char -- ^ The replication protocol being used
> + , ioFlags :: String -- ^ The input/output flags
> + , performanceIndicators :: PerformanceIndicators -- ^ Performance
> indicators
> + , syncStatus :: Maybe SyncStatus -- ^ The status of the syncronization
> of
> + -- the disk (only if it is happening)
> + , resync :: Maybe AdditionalInfo -- ^ Additional info by DRBD 8.0
> + , actLog :: Maybe AdditionalInfo -- ^ Additional info by DRBD 8.0
> + } deriving (Eq, Show)
I don't like something about this data type, but not sure why.
The first note would be that both members hold the minor index, but one
of them is declared as normal constructor whereas the other one as
record. Declaring both as record means you could safely always use
'minorNumber' against a device, whatever value it has. Alternatively,
extracting the minor out might make sense as well.
The second part is that ADTs + records do not play very nicely together
in Haskell (the record system is due to an overhaul for ages now). It
would be better to have a separate data type for all the flags/etc,
e.g.:
data ActiveDevice = ActiveDevice { minorNumber, connectionState, etc. }
and then
data DeviceInfo = UnconfiguredDevice | ConfDevice ActiveDevice
The rationale for this is that now you could accidentally ask "resync
(UnconfiguredDevice 0)", which leads to a runtime error; there is a
warning for it but we didn't enable it yet, because then you can't use
accessor functions ever (with multi-constructor types).
This leaves some questions about how do we deal with the minor index,
but would be somewhat cleaner.
…
> +-- | Data type containing data about the synchronization status of a device.
> +data SyncStatus =
> + SyncStatus
> + { percentage :: Double -- ^ Percentage of syncronized data
> + , partialSyncSize :: Int -- ^ Numerator of the fraction of synced data
> + , totalSyncSize :: Int -- ^ Denominator of the fraction of synced data
> + , syncUnit :: SizeUnit -- ^ Measurement unit of the previous fraction
> + , timeToFinish :: Time -- ^ Expected time before finishing the
> syncronization
> + , speed :: Int -- ^ Speed of the syncronization
> + , want :: Maybe Int -- ^ Want of the syncronization
> + , speedSizeUnit :: SizeUnit -- ^ Size unit of the speed
> + , speedTimeUnit :: TimeUnit -- ^ Time unit of the speed
> + } 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)
Hmm, some things are using Int, whereas you use explicitly Integer. Do
you expect 'seconds' to overflow a 32-bit integer, or altenatively, are
you use totalSyncSize won't overflow one?
thanks,
iustin
