[MirageOS-devel] Error handling in Mirage

[Thomas Leonard <talex5@xxxxxxxxx>]

On 9 July 2014 09:32, Dave Scott <Dave.Scott@xxxxxxxxxx> wrote:
>
> On 8 Jul 2014, at 23:46, Anil Madhavapeddy <anil@xxxxxxxxxx> wrote:
>
>> On 7 Jul 2014, at 10:08, Thomas Leonard <talex5@xxxxxxxxx> wrote:
>>
>>> I'm writing a test Mirage service for queuing files. It's not using
>>> ThomasG's merge-queues - I'm just trying the low-level FS stuff first.
>>>
>>> Some problems I found:
[...]
>>> This means my unikernel only works with Fat. If I don't do this, I
>>> can't report or log error messages. I guess this is also why the test
>>> above gave such a useless error. Are there any plans to make error
>>> handling easier?
>>
>> Dave, what are your thoughts on this?  That is indeed a distinctly
>> abstract-looking block device error :-)
>
> I still find the error handling a bit awkward, particularly when you have 
> layered devices and have wrapped errors. I think pretty-printing will 
> definitely help though!

I found it awkward too. What's the rationale behind Mirage's current
error handling?

Using polymorphic variants can be useful as it forces the user to
handle each possible error case. But that only works if you keep false
positives low. Otherwise, people invent various operators (I've seen
>>|= and >>== so far) to ignore/propagate all errors and any benefits
are lost.

For example, in 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 *)
  ]

  val read: t -> int64 -> page_aligned_buffer list -> [ `Error of
error | `Ok of unit ] io

None of these cases makes sense to me.

An `Unknown can only be logged or displayed, so it would be best to
handle it once in a catch block at the top level, not for every read.
Making the payload be of type exn rather than string would allow code
to handle individual cases if needed (e.g. a remote block device might
return Network_error, indicating that the client should reconnect and
try again).

`Unimplemented and `Disconnected presumably indicate bugs. For most
programs, it would only make sense to report them at the top level.

`Is_read_only is irrelevant for read operations.

I think it would be simpler if exceptions were used and the type
signature for read was just:

  val read: t -> int64 -> page_aligned_buffer list -> unit io

For write, handling `Is_read_only might make sense, but it still seems
better to me to use an exception. After all, if the device is
read-only then you can't get it into a bad state, nor perform any
recovery on it.


In summary, I propose:

- We should separate errors the caller should usually check for vs
bugs/internal errors.

- Probably exceptions should be thrown in the second case. OCaml
programs already have to handle exceptions anyway. If not, `Unknown
should at least take an exn payload, not a string (print handlers
should be registered for any exceptions, of course).

- For cases the caller should check, each function should declare its
own set of errors. DEVICE.read should not return `Is_read_only,
FS.format should not return `File_already_exists, etc.

Thoughts? Apologies if this has all been discussed to death before.
I've did find 
https://lists.cam.ac.uk/pipermail/cl-mirage/2012-March/msg00003.html,
which notes that Async doesn't cope with exceptions. Presumably that
could be wrapped away, though. It also suggests that there is some
problem with Lwt and backtraces, but I'm not sure what the issue is
there. These days, OCaml seems to keep track of when exceptions are
reraised and indicates this in the backtrace.


-- 
Dr Thomas Leonard        http://0install.net/
GPG: 9242 9807 C985 3C07 44A6  8B9A AE07 8280 59A5 3CC1
GPG: DA98 25AE CAD0 8975 7CDA  BD8E 0713 3F96 CA74 D8BA

_______________________________________________
MirageOS-devel mailing list
MirageOS-devel@xxxxxxxxxxxxxxxxxxxx
http://lists.xenproject.org/cgi-bin/mailman/listinfo/mirageos-devel