Summary: Re: [PATCH for-4.17 v3 00/15] OCaml fixes for Xen 4.17

[Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>]

Nothing here is critical enough to go into 4.17 at this juncture.

Various notes/observations from having spent a day trying to untangle
things.

1) Patches 5/6 are a single bugfix and need merging.  Except there was
also an error when taking feedback from the list, and the net result
regresses the original optimisation.  I have a fix sorted in my local queue.

2) The indentation fix (not attached to this series) should scope the
logic, not delete a debug line which was presumably added for a good
reason.  I've got a fix to this effect in my local queue, and we can
discuss the pros/cons of the approach in due course.

3) Patch 1, evtchn Ocaml 5.0 compat, is still missing some corrections
which I gave on earlier postings.  I've fixed it up locally in my queue.

I also notice, while reviewing the whole, that stub_eventchn_init()
passes NULL as a logger, which has the side effect of libxenevtchn
instantiating a default logger which takes control of stdout/stderr. 
Without starting the fight over toxic library behaviour yet again, it
occurs to me in the context of Patch 13, uncaught exception handler,
that in oxenstored, any logging from the C level needs to end up elsewhere.

While we do have ocaml bindings for xentoollog, nothing uses it, and
none of the other libraries (save xl, which isn't used) have a way of
passing the Ocaml Xentoollog down.  This probably wants rethinking, one
way or another.

4) Patches 2/3.  All these libraries have far worse problems than
evtchn, because they can easily use-after-free.  They all need to be
Custom with a finaliser.

5) Patch 4.  The commit message says "A better solution is being worked
on for master", but this is master.  Also, it's not a prerequisite for a
security fix; merely something to make a developers life easier.

6) The re-indent patch.  Policies of when to do it aside, having tried
using it, the format adjustment is incomplete (running ocp-indent gets
me deltas in files I haven't touched), and there needs to be some
.gitignore changes.

That said, it is usually frowned upon to have logic depending on being
in a git tree.  This was perhaps a bigger deal back when we used hg by
default and mirrored into multiple SCMs, but it's still expected not to
rely on this.

7) Patch 8, evtchn fdopen, is two separate patches.  One adding fdopen,
and one adding a NOCLOEXEC argument to the existing init.

They want splitting in two.  fdopen() ought to pass flags so we don't
have to break the ABI again when there is a flag needing passing, and
cloexec probably shouldn't be a boolean.  We should either pass a raw
int32, or a list-of-enums like we do in the xenctrl stubs.  Also, this
patch has inherited errors from patch 1.

9) Patches 8 thru 15 need to be the other side of the intent patch,
because they need backporting to branches which will never get it.  This
is why bugfixes always go at the head of a patch series, and
improvements at the tail.

10) Patch 12 talks about default log levels, but that's bogus
reasoning.  The messages should be warnings because they non-fatal
exceptional cases.

11) Patch 14 talks about using caml_stat_strdup(), but doesn't.

~Andrew