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

[Edwin Torok <edvin.torok@xxxxxxxxxx>]

> On 11 Nov 2022, at 20:46, Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx> wrote:
> 
> Nothing here is critical enough to go into 4.17 at this juncture.

Agreed

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

Thanks.


> 
> 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.


I deleted the debug line to avoid reindenting code, which was frowned upon.
Either way it is fine for me.


> 
> 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.

We should probably start by reviewing the xentoollog bindings, if they never 
got used they're probably buggy.
I think Pau might have some xentoollog bindings though.

> 
> 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.

Indeed, the bindings aren't very safe, and that should be fixed separately.
I've got some patches somewhere to stop the mmap bindings from segfaulting on 
invalid data,
but I lost track whether that got commited or still in one of my local branches.

> 
> 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.

It is to avoid having to add Makefile changes in each security patch commit 
(potentially).
Perhaps the commit message can be changed to say this is the minimal change to 
unbreak the build system,
and a more comprehensive solution is being worked on (using Dune, or dune 
generated makefiles).

> 
> 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),

Perhaps we need to use the strict_comments setting, I think it tries to leave 
comments untouched,
but that also means the outcome depends on the starting state.
And I hope it doesn't depend on ocp-indent version.

> 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.

We could use 'find' instead.

> 
> 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

The ABI changes everytime a new variant is added (the interface hash will 
change, and you need to rebuild/relink),
so using a single flag variant doesn't avoid ABI changes like it would in C.

> when there is a flag needing passing, and
> cloexec probably shouldn't be a boolean.

The preferred way to bind CLOEXEC in OCaml is to use a boolean, see
Unix.html <https://v2.ocaml.org/api/Unix.html>, in particular
`val socket : ?cloexec:bool ->
socket_domain -> socket_type -> int -> file_descr`
Perhaps I should've spelled this out in the commit message.

>   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 was done on purpose like this to ensure that indentation is backported in 
some way,
because the lack of indentation has previously broken backports/rebases (see 
the debug line introduced in the wrong place in the live update patch).
Without a comprehensive testsuite (which is being worked on, but not ready yet) 
it is then impossible to tell whether a backport is correct or not,
even if it compiles, it may have some things in the wrong place, and wrong 
indentation just makes reviewing those more difficult.

Otherwise I have to keep making changes with the wrong indentation or avoid 
indentation changes in patches, which has previously introduced bugs.
It is extra work, and all it does is decrease the quality of the end result and 
confuse both patch authors and reviewers.
Furthermore the indentation commit on its own is separate and can be proven to 
have no functional changes if you view the diff ignoring whitespace.

We first need to make oxenstored suitable to be developed on, which means 
starting at the basics, fixing up indentation, build systems
(all the bugs in the bindings you pointed out), etc.
I tried my best to avoid making changes like this within the XSA fix (which 
only contributed to lengthening the time to develop it),
but now that we're no longer constrained by XSA rules we should fix things the 
right way.

Keeping the status quo just for the sake of it only discourages contribution to 
oxenstored.

If it helps we can consider all past versions of oxenstored unsupported (by me) 
and support only master going forward, and once we have master in a reasonable 
shape
we can decide what and how to backport, and which versions to reinstate support 
on. That would avoid placing artificial constraints on master development.
I intend to change master substantially enough that most backports will be 
impossible unless you take an almost entirely new version of oxenstored.

In fact releases of oxenstored shouldn't be tied to a particular hypervisor 
version: there should be a single long term supported stable branch of 
oxenstored and a master branch.
(I understand that is not possible yet due to the use of the 2 unstable xenctrl 
APIs, one of which has an outstanding patch series to remove the dependency)
The current situation only creates the illusion of support for the backported 
versions, because there is no (comprehensive) testsuite to check that those 
backports work,
and XenServer only tests internally 4.13 and latest master, anything inbetween 
is technically untested, and may be more buggy than just running one version.

>   This
> is why bugfixes always go at the head of a patch series, and
> improvements at the tail.

I agree that is how it should generally be, but that means improvements can 
never be done because we always keep finding more and more bugs as we do 
improvements,
and then do a lot of risky rebases to get patches moved ahead, and without 
minimal things like automated tools to keep at least indentation consistent
the end result is a mess that cannot be trusted.


> 
> 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.

The commit message for this is fixed on my github branch: 
https://github.com/edwintorok/xen/commit/dc893a079fd7cf2a9bb8ed03cca3d249a53e3c44
It initially had that function, but it is not available in 4.02.3 

Thanks for helping sort out the patch series.

Best regards,
--Edwin