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

On the other hand here is a bug I just spotted when looking at indentation 
changes (or rather reading the code after the indentation change),
and there are probably more:

```
        if not (Domain.get_io_credit dom > 0) then
            debug "Looking up domid %d" (Domain.get_id dom);
-           let con = Connections.find_domain cons (Domain.get_id dom) in
-           if not (Connection.has_more_work con) then (
-               Process.do_output store cons domains con;
-               Process.do_input store cons domains con;
-               if Connection.has_more_work con then
-                   (* Previously thought as no work, but detect some after 
scan (as
-                      processing a new message involves multiple steps.) It's 
very
-                      likely to be a "lazy" client, bump its credit. It could 
be false
-                      positive though (due to time window), but it's no harm 
to give a
-                      domain extra credit. *)
-                   let n = 32 + 2 * (Domains.number domains) in
-                   info "found lazy domain %d, credit %d" (Domain.get_id dom) 
n;
-                   Domain.set_io_credit ~n dom
-           ) in
+       let con = Connections.find_domain cons (Domain.get_id dom) in
+       if not (Connection.has_more_work con) then (
+           Process.do_output store cons domains con;
+           Process.do_input store cons domains con;
+           if Connection.has_more_work con then
+               (* Previously thought as no work, but detect some after scan (as
+                  processing a new message involves multiple steps.) It's very
+                  likely to be a "lazy" client, bump its credit. It could be 
false
+                  positive though (due to time window), but it's no harm to 
give a
+                  domain extra credit. *)
+               let n = 32 + 2 * (Domains.number domains) in
+               info "found lazy domain %d, credit %d" (Domain.get_id dom) n;
+               Domain.set_io_credit ~n dom
+       ) in
```

Notice how all that code "seems" to be inside the if unless you read really 
closely, but in fact it isn't, just the debug statement is.
Which means whenever I reviewed this code (to look for performance or security 
bugs) I've been reading it wrong the same way the original author got it wrong 
when indenting it.
In this case the original author being me, as I've introduced this bug in 
42f0581a91d4340ae66768a29fd779f83415bdfe back in 2021, where prior to the 
change in that commit indentation was correct,
but the patch added the 'debug' line in the wrong place (before the let instead 
of after it, and had I had my usual tools available to indent the file correctly
this problem would've been detected and corrected before commiting the bug into 
the codebase...
And was probably a side-effect of trying not to reindent the code to reduce the 
patch size for the security fix, and by doing so introducing an actual 
functional bug
)
(And I've recently fixed a similar bug elsewhere in XAPI, in which case I 
wasn't the original author of such a bug)
Indentation can't really be trusted to humans :)

(It means that even if a domain already has IO credit we still scan its ring 
for more work)

So some indentation changes will probably come in as bugfixes for 4.17.1 (well 
maybe not reindenting the whole file, just the problematic region of 
code/function).

Best regards,
--Edwin