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