> On 9 Nov 2022, at 14:18, Edwin Torok <edvin.to...@citrix.com> wrote:
>
>
>
>> On 9 Nov 2022, at 02:40, Henry Wang <henry.w...@arm.com> wrote:
>>
>> Hi Julien,
>>
>>> -----Original Message-----
>>> From: Julien Grall <jul...@xen.org>
>>> 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
