On Tue, Nov 20, 2012 at 12:10 PM, 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
>
> 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.
>
I agree, and I think I would go for declaring both as records. I really
don't like how easy it is in Haskell to create things without names. It is
really handy, but I guess it becomes confusing when you go back and look at
your code (or, even worse, somebody else's) after a while.
> 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.
>
I see... I had no idea about this.
>
> …
>
> > +-- | 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?
>
You're right, they should be reversed.
I will probably send a patch for this soon.
Thanks,
Michele
