Re: [PATCH for-4.17 v3 07/15] CODING_STYLE(tools/ocaml): add 'make format' and remove tabs

[Edwin Torok <edvin.torok@xxxxxxxxxx>]

> On 9 Nov 2022, at 14:18, Edwin Torok <edvin.torok@xxxxxxxxxx> wrote:
> 
> 
> 
>> On 9 Nov 2022, at 02:40, Henry Wang <Henry.Wang@xxxxxxx> wrote:
>> 
>> Hi Julien,
>> 
>>> -----Original Message-----
>>> From: Julien Grall <julien@xxxxxxx>
>>> Subject: Re: [PATCH for-4.17 v3 07/15] CODING_STYLE(tools/ocaml): add
>>> 'make format' and remove tabs
>>> While I understand the goal and support, this seems to be a bit too late
>>> to do it in Xen 4.17 (we are only a couple of weeks away). At this stage
>>> of the release we should only do bug fix.
>>> 
>>> This is clearly only a comesmetic change and there I would argue this
>>> should be deferred to 4.18. That said the last call is from the RM.
>> 
>> I agree with your point. I think maybe defer the patch to 4.18 is better,
>> given the deep freeze state we are currently in.
> 
> 
> Indentation can't really be trusted to humans :)
> 


It might be better to consider oxenstored unsupported in 4.17 at this point and 
try again for 4.17.1 or 4.18, it is probably too late to fix up all the bugs 
that I keep finding in it in a way in which it can be (security) supported, 
although I thought the goal of a release candidate is to try and find bugs and 
fix them before release.

But doing that while also trying to keep whitespace and indentation changes out 
of the patches (or trying to keep the patches small), is more likely to 
introduce more bugs into it at this point rather than fix things.

I was not able to do or send any of these patches sooner because the XSA-326 
and XSA-115/etc. work in the past years has prevented any other kind of work 
from being sent (it'd have made rebasing the XSA series more difficult, and/or 
reveal the XSA as part of various other changes and commits), it is unfortunate 
that there is a release quite so close after an oxenstored XSA

I tried fixing up the rest of the series to not depend on this patch, but 
without an actual 'make format' rule to give it consistent indentation
it is just too error-prone or risky to fix it up at this point (I don't really 
mind whether indentation is tabs vs spaces, it is the inconsistency and 
non-automated nature of it that is the problem),
e.g. it is quite easy to accidentally duplicate code, or get the code in the 
wrong scope, or introduce bugs like the one I just mentioned before when a new 
statement gets inserted and the code seemingly looks ok but its semantics is 
entirely changed (and that is hidden by the inconsistent/non-automated 
indentation).


Perhaps by trying to merge some of the changes/fixes from 
https://github.com/mirage/ocaml-xenstore and getting that production ready, 
which is a much better codebase to start from than
the current one in the Xen tree.:
* it has actual unit tests (which I tried to introduce as part of a security 
fix for the intree version of oxenstored but got repeatedly discouraged from 
including it to not make the security fix too large)
* it was not vulnerable XSA-353
* it has fixed part of XSA-326 together with a unit test nearly 10 years before 
Xen project rediscovered the same bug and realized it is a security bug in the 
in-tree version: 
https://github.com/mirage/ocaml-xenstore/commit/21e96654c27c01cf52e5d7aabc5ee53e07f2cbb7
* (of course mirage version has never been used in production so it is entirely 
likely it has *different* bugs)
* in-tree Xen is difficult to contribute to: 
  * it has a broken build system that keeps throwing 'inconsistent assumptions'
  * it has inconsistent and as we can see sometimes wrong indentation
  * patches get posted to the mailing list and sometimes lost (e.g. 
https://patchwork.kernel.org/project/xen-devel/list/?series=339731&archive=both 
is still not committed), and I'm fairly sure I've seen an ack somewhere, but 
patchwork can't find it now

So I think oxenstored in 4.18+ will likely be sufficiently different than 4.17 
that direct backports won't be possible anyway (indentation changes or not), so 
having this indentation patch in 4.17 wouldn't really help much.
The disadvantage is that we lose all the bugfixes in the patch series after the 
indentation change, but if we consider oxenstored unsupported in 4.17 that may 
not matter.

I can resend this patch series for master without a 'for-4.17 tag' and keep 
doing development there.

Best regards,
--Edwin