On Tue, Oct 4, 2016 at 12:04 PM, Anil Madhavapeddy <a...@recoil.org> wrote:
> On 1 Oct 2016, at 21:49, Hannes Mehnert <han...@mehnert.org> wrote: > > > > On 30/09/2016 17:13, Hannes Mehnert wrote: > >> If nobody disagrees (and comes up with non-contrived examples), I'm > >> happy to massage code into this direction within the next few days. > > > > Thanks for the feedback, I prepared a bunch of PRs linked from > > https://github.com/mirage/mirage/pull/602 . Lots of CI failures are due > > to the fact that the repository needs to pin another library, thus they > > should solve themselves. I built most of the packages (esp tcpip, > > mirage-block-xen&unix) locally and run the testsuites successfully. > > > > Of special interest might be the PR to mirage-platform and mirage-solo5, > > which catch the exception from startup and emit a log message to > Logs.err. > > > > Since this is a change affecting lots of repositories, it would be great > > to have this change merged rather sooner than later (mirage-dev likely > > needs to be extended with some of these packages). If anyone is up for > > merging & adjusting mirage-dev, feel free to do so, I need some sleep. > > I've started porting some of the fallout from this (e.g. qcow-format), and > immediately ran into some of the other module types such as FLOW.write > returning polymorphic `Error|`Ok pairs. > > So this leads me to wonder if we shouldn't just bite the bullet and port > all these interfaces to use Rresult [1] combinators instead. It does > provide > excellent support for propagating errors, and combinators for mapping > over result types quite conveniently. In the case of connect, if we made > it a `('a,'b) result io` return value we would end up with a nice > ready-made > error string that can be printed, instead of having to catch the Lwt.fail > exception and turn it into a string ourself. > > It would be a lot of breakage in the short term and probably delay the > release by a couple of weeks, but in return it would significantly increase > the succinctness of code (particularly in the filesystem layer). > > Thoughts? > If we're up for a bit of temporary breakage then I'm very keen to move some of the type definitions out of the V1 signatures so we don't have to cut and paste them all over the place. For example with `BLOCK` we have: type error = [ | `Unknown of string (** an undiagnosed error *) | `Unimplemented (** operation not yet implemented in the code *) | `Is_read_only (** you cannot write to a read/only instance *) | `Disconnected (** the device has been previously disconnected *) ] ... type info = { read_write: bool; (** True if we can write, false if read/only *) sector_size: int; (** Octets per sector *) size_sectors: int64; (** Total sectors per device *) } ... Since these types don't refer to any abstract types in the signature I think it's pointless having them there. It would also be good to replace [`Unknown of string] and probably [`Unimplemented] with the [`Msg of string] as recommended by the Rresult docs-- I don't think either error can really be handled. Perhaps [`Disconnected] falls into the same category since it indicates a bug in the application (use after [disconnect]). Of those 4 errors perhaps only [`Is_read_only] is worth keeping since it refers to something in the environment rather than a bug in the code. WDYT? Cheers, Dave > > [1] http://erratique.ch/software/rresult/doc/Rresult.html > > -a > > _______________________________________________ > MirageOS-devel mailing list > MirageOS-devel@lists.xenproject.org > https://lists.xenproject.org/cgi-bin/mailman/listinfo/mirageos-devel > -- Dave Scott
_______________________________________________ MirageOS-devel mailing list MirageOS-devel@lists.xenproject.org https://lists.xenproject.org/cgi-bin/mailman/listinfo/mirageos-devel
