| Message ID | cover.1667920496.git.edvin.torok@citrix.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | OCaml fixes for Xen 4.17 | expand |
Hi, On 08/11/2022 15:33, Edwin Török wrote: > See CODING_STYLE: Xen uses spaces, not tabs. > > * OCaml code: > > Using `ocp-indent` for now to just make minimal modifications in > tabs vs spaces and get the right indentation. > We can introduce `ocamlformat` later. > > * C stubs: > > just replace tabs with spaces now, using `indent` or `clang-format` > would change code too much for 4.17. > > This avoids perpetuating a formatting style that is inconsistent with > the rest of Xen, and that makes preparing and submitting patches more > difficult (OCaml indentation tools usually only support spaces, not tabs). > > Contains a bugfix for `abi-check` script to handle the change in the > amount of whitespace. > > No functional change. > > Signed-off-by: Edwin Török <edvin.torok@citrix.com> > > -- > Reason for inclusion in 4.17: > - makes it easier to backport changes from master to 4.17 Right, but you will have the problem when backporting to 4.16 and older. So the overhead will always be there for a couple of years. > - avoid perpetuating a different coding style (I thought tabs were > mandated by Xen, and was about to fix up my editor config to match > when I realized Xen already mandates the use of spaces) > - should make submitting patches for OCaml easier (OCaml indentation > tools know only about spaces, so I either can't use them, or have to > manually adjust indentation every time I submit a patch) > - it can be verified that the only change here is the Makefile change > for the new rule, 'git log -p -1 -w' should be otherwise empty 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. Cheers,
> On 8 Nov 2022, at 16:03, Julien Grall <julien@xen.org> wrote: > > Hi, > > On 08/11/2022 15:33, Edwin Török wrote: >> See CODING_STYLE: Xen uses spaces, not tabs. >> * OCaml code: >> Using `ocp-indent` for now to just make minimal modifications in >> tabs vs spaces and get the right indentation. >> We can introduce `ocamlformat` later. >> * C stubs: >> just replace tabs with spaces now, using `indent` or `clang-format` >> would change code too much for 4.17. >> This avoids perpetuating a formatting style that is inconsistent with >> the rest of Xen, and that makes preparing and submitting patches more >> difficult (OCaml indentation tools usually only support spaces, not tabs). >> Contains a bugfix for `abi-check` script to handle the change in the >> amount of whitespace. >> No functional change. >> Signed-off-by: Edwin Török <edvin.torok@citrix.com> >> -- >> Reason for inclusion in 4.17: >> - makes it easier to backport changes from master to 4.17 > > Right, but you will have the problem when backporting to 4.16 and older. So the overhead will always be there for a couple of years. There will always be more than one Xen release in support, which means we'd never be able to fix this. At some point we need to decide to go ahead and just do it (which may or may not be 4.17) The whitespace fixup needs to happen somewhere anyway, there are a couple of options: * use git rebase/cherry-pick -Xignore-space-change to avoid backport failing to apply * do a reformat on 4.16 branch too (backport the couple of lines of Makefile change and run 'make format' and commit the result) * include the reformat as a prerequisite of the next security patch (I'd prefer not to, hence including it in 4.17) * do the whitespace fixup prior to submission, turning a correctly formatted patch with spaces into an incorrectly formatted one with tabs. I wouldn't have minded doing this if the coding style said tabs, I could've run my reformat tool locally and fix it up with 'sed' prior to submission. However this seems unnecessary overhead if doing this works actively against the existing coding style. However if we put this into 4.17 and there are no new security issues discovered before 4.16 goes out of support then we'll be in a much better position for 4.18 onward. There is still quite a lot of work outstanding in testing of oxenstored (updating my fuzzer for one), so I can't really predict at this point whether there are more security bugs in oxenstored or not, so maybe we need to take this decision at 4.18 time? Not sure. > >> - avoid perpetuating a different coding style (I thought tabs were >> mandated by Xen, and was about to fix up my editor config to match >> when I realized Xen already mandates the use of spaces) >> - should make submitting patches for OCaml easier (OCaml indentation >> tools know only about spaces, so I either can't use them, or have to >> manually adjust indentation every time I submit a patch) >> - it can be verified that the only change here is the Makefile change >> for the new rule, 'git log -p -1 -w' should be otherwise empty > > 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. I think it can be fairly easily proven that there is no functional change by rerunning the make format command manually, and by looking at the diff with ignore whitespace as suggested above. I understand the reluctance in including it (which is why I was not sure whether to post it in the first place), but I think it might be beneficial to do it. There is a large backlog of work in oxenstored that got piled up during the past couple of years of XSA work, and it'd be a lot easier to update and upstream those if we wouldn't have to worry about indentation at all. Usually patches on LCM and security branches are avoided to reduce the risk of breaking anything, but a reindentation patch should not really break anything (well other than the abi-check script in the build, but I fixed that to accept both ways). One alternative would be that I add another step after reformat that runs sed and turns spaces back into tabs for now, and that way I can still run 'make format' at each step while preparing patches for master, or 4.17 or security patches and get something consistent, and that minimizes other whitespace changes, but it wouldn't completely eliminate them (e.g. there are pieces of code that are just wrongly indented, so there'd be at least a diff to fix all that). > > 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. Best regards, --Edwin > > Cheers, > > > -- > Julien Grall
On 08/11/2022 17:02, Edwin Torok wrote: > > >> On 8 Nov 2022, at 16:03, Julien Grall <julien@xen.org> wrote: >> >> Hi, >> >> On 08/11/2022 15:33, Edwin Török wrote: >>> See CODING_STYLE: Xen uses spaces, not tabs. >>> * OCaml code: >>> Using `ocp-indent` for now to just make minimal modifications in >>> tabs vs spaces and get the right indentation. >>> We can introduce `ocamlformat` later. >>> * C stubs: >>> just replace tabs with spaces now, using `indent` or `clang-format` >>> would change code too much for 4.17. >>> This avoids perpetuating a formatting style that is inconsistent with >>> the rest of Xen, and that makes preparing and submitting patches more >>> difficult (OCaml indentation tools usually only support spaces, not tabs). >>> Contains a bugfix for `abi-check` script to handle the change in the >>> amount of whitespace. >>> No functional change. >>> Signed-off-by: Edwin Török <edvin.torok@citrix.com> >>> -- >>> Reason for inclusion in 4.17: >>> - makes it easier to backport changes from master to 4.17 >> >> Right, but you will have the problem when backporting to 4.16 and older. So the overhead will always be there for a couple of years. > > > There will always be more than one Xen release in support, which means we'd never be able to fix this. Note that I haven't said this should never be done. This just need to be correctly timed. Doing it in the middle of a deep freeze doesn't look right to me. [...] >>> - avoid perpetuating a different coding style (I thought tabs were >>> mandated by Xen, and was about to fix up my editor config to match >>> when I realized Xen already mandates the use of spaces) >>> - should make submitting patches for OCaml easier (OCaml indentation >>> tools know only about spaces, so I either can't use them, or have to >>> manually adjust indentation every time I submit a patch) >>> - it can be verified that the only change here is the Makefile change >>> for the new rule, 'git log -p -1 -w' should be otherwise empty >> >> 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. > > I think it can be fairly easily proven that there is no functional change by rerunning the make format command manually, and by looking at the diff with ignore whitespace as suggested above. That's not really the point here. The point is that if we start to allow large coding style change (whether automatic or manual) very late in the release then it will be hard to reject it in the future. In fact we already have guidelines for that. If you look at [1], only bug fixes should be done past the code freeze (23rd September). As I wrote before, this patch only seem to be a cosmetic/quality improvement. IOW this is not a bug fix and would not qualify for 4.17. > I understand the reluctance in including it (which is why I was not sure whether to post it in the first place), but I think it might be beneficial to do it. > There is a large backlog of work in oxenstored that got piled up during the past couple of years of XSA work, and it'd be a lot easier to update and upstream those if we wouldn't have to worry > about indentation at all. This is an argument for including this patch in Xen 4.18. As I wrote above, I am not against that. > > Usually patches on LCM and security branches are avoided to reduce the risk of breaking anything, but a reindentation patch should not really break anything (well other than the abi-check script in the build, but I fixed that to accept both ways). What does LCM stands for? > > One alternative would be that I add another step after reformat that runs sed and turns spaces back into tabs for now, and that way I can still run 'make format' at each step while preparing patches for master, or 4.17 or security patches and get something consistent, and that minimizes other whitespace changes, but it wouldn't completely eliminate them (e.g. there are pieces of code that are just wrongly indented, so there'd be at least a diff to fix all that). I would view this as a feature. Which again doesn't qualify for Xen 4.17 release. This doesn't mean the patch couldn't be backported afterwards. Cheers, [1] AS8PR08MB7991145C8063D6939AFFED8F92829@AS8PR08MB7991.eurprd08.prod.outlook.com > > > Best regards, > --Edwin > >> >> Cheers, > > >> >> >> -- >> Julien Grall >
> On 8 Nov 2022, at 17:26, Julien Grall <julien@xen.org> wrote: > > > > On 08/11/2022 17:02, Edwin Torok wrote: >>> On 8 Nov 2022, at 16:03, Julien Grall <julien@xen.org> wrote: >>> >>> Hi, >>> >>> On 08/11/2022 15:33, Edwin Török wrote: >>>> See CODING_STYLE: Xen uses spaces, not tabs. >>>> * OCaml code: >>>> Using `ocp-indent` for now to just make minimal modifications in >>>> tabs vs spaces and get the right indentation. >>>> We can introduce `ocamlformat` later. >>>> * C stubs: >>>> just replace tabs with spaces now, using `indent` or `clang-format` >>>> would change code too much for 4.17. >>>> This avoids perpetuating a formatting style that is inconsistent with >>>> the rest of Xen, and that makes preparing and submitting patches more >>>> difficult (OCaml indentation tools usually only support spaces, not tabs). >>>> Contains a bugfix for `abi-check` script to handle the change in the >>>> amount of whitespace. >>>> No functional change. >>>> Signed-off-by: Edwin Török <edvin.torok@citrix.com> >>>> -- >>>> Reason for inclusion in 4.17: >>>> - makes it easier to backport changes from master to 4.17 >>> >>> Right, but you will have the problem when backporting to 4.16 and older. So the overhead will always be there for a couple of years. >> There will always be more than one Xen release in support, which means we'd never be able to fix this. > > Note that I haven't said this should never be done. This just need to be correctly timed. Doing it in the middle of a deep freeze doesn't look right to me. > > [...] > >>>> - avoid perpetuating a different coding style (I thought tabs were >>>> mandated by Xen, and was about to fix up my editor config to match >>>> when I realized Xen already mandates the use of spaces) >>>> - should make submitting patches for OCaml easier (OCaml indentation >>>> tools know only about spaces, so I either can't use them, or have to >>>> manually adjust indentation every time I submit a patch) >>>> - it can be verified that the only change here is the Makefile change >>>> for the new rule, 'git log -p -1 -w' should be otherwise empty >>> >>> 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. >> I think it can be fairly easily proven that there is no functional change by rerunning the make format command manually, and by looking at the diff with ignore whitespace as suggested above. > > That's not really the point here. The point is that if we start to allow large coding style change (whether automatic or manual) very late in the release then it will be hard to reject it in the future. > > In fact we already have guidelines for that. If you look at [1], only bug fixes should be done past the code freeze (23rd September). > > As I wrote before, this patch only seem to be a cosmetic/quality improvement. IOW this is not a bug fix and would not qualify for 4.17. > >> I understand the reluctance in including it (which is why I was not sure whether to post it in the first place), but I think it might be beneficial to do it. >> There is a large backlog of work in oxenstored that got piled up during the past couple of years of XSA work, and it'd be a lot easier to update and upstream those if we wouldn't have to worry >> about indentation at all. > > This is an argument for including this patch in Xen 4.18. As I wrote above, I am not against that. > >> Usually patches on LCM and security branches are avoided to reduce the risk of breaking anything, but a reindentation patch should not really break anything (well other than the abi-check script in the build, but I fixed that to accept both ways). > > What does LCM stands for? LCM is what we internally call the long term release branch where we backport fixes (i.e. 4.16 is an LCM branch after it is released). I think it'd be equivalent to the stable branch of Xen/Linux, I should've used that terminology. (I think it is an abbreviation of software lifecycle management) > >> One alternative would be that I add another step after reformat that runs sed and turns spaces back into tabs for now, and that way I can still run 'make format' at each step while preparing patches for master, or 4.17 or security patches and get something consistent, and that minimizes other whitespace changes, but it wouldn't completely eliminate them (e.g. there are pieces of code that are just wrongly indented, so there'd be at least a diff to fix all that). > > I would view this as a feature. Which again doesn't qualify for Xen 4.17 release. This doesn't mean the patch couldn't be backported afterwards. Ah ok, if this can be backported to 4.17.1, or applied for 4.18 that might work too. (I just thought the rules around backport would be even stricter than what can go into a release) I'll try to find a short term workaround for the spaces vs tabs issues meanwhile. > > Cheers, > > [1] AS8PR08MB7991145C8063D6939AFFED8F92829@AS8PR08MB7991.eurprd08.prod.outlook.com Hmm I thought that is my Outlook rewriting the link, but the archive at lore.kernel.org seems to have this mangled URL as well which I cannot open. Could you send it in such a way that it is not encoded when being sent (e.g. base64 encode it...) > >> Best regards, >> --Edwin >>> >>> Cheers, >>> >>> >>> -- >>> Julien Grall > > -- > Julien Grall
Hi Julien, > -----Original Message----- > From: Julien Grall <julien@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. Kind regards, Henry > > Cheers, > > -- > Julien Grall
Hi Edwin, > -----Original Message----- > > [1] > AS8PR08MB7991145C8063D6939AFFED8F92829@AS8PR08MB7991.eurprd08 > .prod.outlook.com > > > Hmm I thought that is my Outlook rewriting the link, but the archive at > lore.kernel.org seems to have this mangled URL as well which I cannot open. > Could you send it in such a way that it is not encoded when being sent (e.g. > base64 encode it...) Appending "https://lore.kernel.org/xen-devel/" before the link from Julien works for me. But I think the link is just the release schedule :) Kind regards, Henry
> On 9 Nov 2022, at 03:19, Henry Wang <Henry.Wang@arm.com> wrote: > > Hi Edwin, > >> -----Original Message----- >>> [1] >> AS8PR08MB7991145C8063D6939AFFED8F92829@AS8PR08MB7991.eurprd08 >> .prod.outlook.com >> >> >> Hmm I thought that is my Outlook rewriting the link, but the archive at >> lore.kernel.org seems to have this mangled URL as well which I cannot open. >> Could you send it in such a way that it is not encoded when being sent (e.g. >> base64 encode it...) > > Appending "https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Fxen-devel%2F&data=05%7C01%7Cedvin.torok%40citrix.com%7C1c7455639df84f970eab08dac2012fce%7C335836de42ef43a2b145348c2ee9ca5b%7C0%7C0%7C638035607613905983%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=l0oQ0815ndwWt4buGasPYYmVV3FdnrC%2F4wKrIoLqki8%3D&reserved=0" before the link from Julien > works for me. But I think the link is just the release schedule :) Thanks, this link works indeed (it is an Outlook message ID after all, not one of Outlook's link rewrites): https://lore.kernel.org/xen-devel/AS8PR08MB7991145C8063D6939AFFED8F92829@AS8PR08MB7991.eurprd08.prod.outlook.com/ Best regards, --Edwin
> On 9 Nov 2022, at 02:40, Henry Wang <Henry.Wang@arm.com> wrote: > > Hi Julien, > >> -----Original Message----- >> From: Julien Grall <julien@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
> On 9 Nov 2022, at 14:18, Edwin Torok <edvin.torok@citrix.com> wrote: > > > >> On 9 Nov 2022, at 02:40, Henry Wang <Henry.Wang@arm.com> wrote: >> >> Hi Julien, >> >>> -----Original Message----- >>> From: Julien Grall <julien@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
> On 9 Nov 2022, at 16:26, Edwin Török <edvin.torok@citrix.com> wrote: > > > >> On 9 Nov 2022, at 14:18, Edwin Torok <edvin.torok@citrix.com> wrote: >> >> >> >>> On 9 Nov 2022, at 02:40, Henry Wang <Henry.Wang@arm.com> wrote: >>> >>> Hi Julien, >>> >>>> -----Original Message----- >>>> From: Julien Grall <julien@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 Ah I see that 'Live Update' for oxenstored is already marked as 'not functional' (I indeed remember seeing a patch to that effect), so looks like nothing else needs to be done here for 4.17. https://xenbits.xen.org/docs/unstable/support-matrix.html I can try fixing that up for master, 4.17.1 or 4.18, whenever that opens. Best regards, --Edwin
> On 9 Nov 2022, at 02:40, Henry Wang <Henry.Wang@arm.com> wrote: > >> >> -----Original Message----- >> From: Julien Grall <julien@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. I disagree. This is an automated change that can be verified to not add functional changes. Edvin has demonstrated that wrong indentation has mislead reviewers in the past and caused bugs. Nobody except Edvin has contributed to the affected code in years and thus it is not a burden on the project outside the OCaml part. I suggest to accept this. — C
Hi Christian, > -----Original Message----- > From: Christian Lindig <christian.lindig@citrix.com> > 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. > > I disagree. This is an automated change that can be verified to not add > functional changes. Edvin has demonstrated that wrong indentation has > mislead reviewers in the past and caused bugs. Nobody except Edvin has > contributed to the affected code in years and thus it is not a burden on the > project outside the OCaml part. I suggest to accept this. I understand points from you, Edwin and Julien, but I think in the earlier discussion in this thread, Julien has provided an argument [1] which I do think is a valid reason to defer this patch a little bit. But since you are the only maintainer of the Ocaml code, so if you strongly insist this patch should be included for the release and there would not be any more explicit objections from others in the next couple of days, I think I will provide my release-ack for the purpose of respecting opinions from the maintainer. Hope this solution should be acceptable to you. [1] https://lore.kernel.org/xen-devel/1f8c90cd-8037-84eb-d6f7-c639f8a87585@xen.org/ Kind regards, Henry > > — C > > >
> On 10 Nov 2022, at 09:25, Henry Wang <Henry.Wang@arm.com> wrote: > > Hi Christian, > >> -----Original Message----- >> From: Christian Lindig <christian.lindig@citrix.com> >> 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. >> >> I disagree. This is an automated change that can be verified to not add >> functional changes. Edvin has demonstrated that wrong indentation has >> mislead reviewers in the past and caused bugs. Nobody except Edvin has >> contributed to the affected code in years and thus it is not a burden on the >> project outside the OCaml part. I suggest to accept this. > > I understand points from you, Edwin and Julien, but I think in the earlier > discussion in this thread, Julien has provided an argument [1] which I do > think is a valid reason to defer this patch a little bit. > > But since you are the only maintainer of the Ocaml code, so if you strongly > insist this patch should be included for the release and there would not be > any more explicit objections from others in the next couple of days, I think I > will provide my release-ack for the purpose of respecting opinions from the > maintainer. Hope this solution should be acceptable to you. Thanks Henry. I think the argument here is the balance between maintaining a policy against late large changes and improving the quality and the reliability of future patches by using more automation. I agree that large functional changes and any change that can’t be verified should be avoided but I don’t think this case is one. However, I am fine deferring the patch based on an agreed policy if we can make it a priority to get in in soon. For me this is part of improving the OCaml code base and project quality by using more automation in formatting and the build system that lowers the barrier for contributors such that they don’t have to worry about procedural aspects like tabs, spaces, indentation, or build systems. — C
Hi Christian, > -----Original Message----- > From: Christian Lindig <christian.lindig@citrix.com> > Subject: Re: [PATCH for-4.17 v3 07/15] CODING_STYLE(tools/ocaml): add > 'make format' and remove tabs > > On 10 Nov 2022, at 09:25, Henry Wang <Henry.Wang@arm.com> wrote: > > > > Hi Christian, > > > >> -----Original Message----- > >> From: Christian Lindig <christian.lindig@citrix.com> > >> 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. > >> > >> I disagree. This is an automated change that can be verified to not add > >> functional changes. Edvin has demonstrated that wrong indentation has > >> mislead reviewers in the past and caused bugs. Nobody except Edvin has > >> contributed to the affected code in years and thus it is not a burden on > the > >> project outside the OCaml part. I suggest to accept this. > > > > I understand points from you, Edwin and Julien, but I think in the earlier > > discussion in this thread, Julien has provided an argument [1] which I do > > think is a valid reason to defer this patch a little bit. > > > > But since you are the only maintainer of the Ocaml code, so if you strongly > > insist this patch should be included for the release and there would not be > > any more explicit objections from others in the next couple of days, I think I > > will provide my release-ack for the purpose of respecting opinions from the > > maintainer. Hope this solution should be acceptable to you. > > Thanks Henry. I think the argument here is the balance between maintaining > a policy against late large changes and improving the quality and the > reliability of future patches by using more automation. I agree that large > functional changes and any change that can’t be verified should be avoided > but I don’t think this case is one. However, > I am fine deferring the patch based on an agreed policy if we can make it a > priority to get in in soon. Thanks for your understanding. I will take a note of this patch and try to ping committers to commit this patch as soon as the staging tree gets unfrozen after the release. In this way I think your concerns in... > For me this is part of improving the OCaml code > base and project quality by using more automation in formatting and the > build system that lowers the barrier for contributors such that they don’t > have to worry about procedural aspects like tabs, spaces, indentation, or > build systems. ...here would be minimized. I do understand your points and frustration. Thanks again for your understanding. Kind regards, Henry > > — C
Nothing here is critical enough to go into 4.17 at this juncture. Various notes/observations from having spent a day trying to untangle things. 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. 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. 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. 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. 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), 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. 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 when there is a flag needing passing, and cloexec probably shouldn't be a boolean. 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 is why bugfixes always go at the head of a patch series, and improvements at the tail. 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. ~Andrew
> On 11 Nov 2022, at 20:46, Andrew Cooper <Andrew.Cooper3@citrix.com> 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
These are the patches that I have outstanding for Xen 4.17. I have included a reason why I'm requesting them to be included in 4.17 after the --- line in each individual patch, see also a summary below. For convenience the patches are also available in a git repo: ``` git remote add edwintorok https://github.com/edwintorok/xen.git git fetch edwintorok private/edvint/for-4.17 git log -p origin/master..private/edvint/for-4.17 ``` And viewable with a browser too: https://github.com/edwintorok/xen/compare/private/edvint/for-4.17 * 3 patches related to OCaml 5 support https://patchwork.kernel.org/project/xen-devel/list/?series=680975 These have already been posted to the list previously, but not yet committed to master (I probably didn't use the correct subject and CC line for patches meant for 4.17, I think I've fixed that now) * Makefile.rules followup Also part of https://patchwork.kernel.org/project/xen-devel/list/?series=680975 these address some review feedback that I received after patches got committed * oxenstored live update bugfixes Testing of oxenstored live update has revealed some bugs (some of which got discovered on the C side too and fixed during one of the previous XSAs, but unfortunately none of that discussion is public, and we've ended up rediscovering the issue in the OCaml implementation too, which reminded me of the XSA discussions at the time). This brings the OCaml live update handling of event channels closer to the C xenstored version. It also fixes a few more bugs regarding logging and exception handling during live update, and during out of memory situations (theoretical now after XSA-326 fix). * a bugfix for a xenctrl binding Xen returns uninitialized data as part of a paging op domctl when a domain is dying. Workaround in the C stub by always initializing the domctl arguments to detect this. Xen fix in hypervisor side will be done separately, but even then having this is useful defensive coding. This is a 9 year old bug that still happens today, I've encountered it while testing this very series, hence the inclusion here. I expect most of these to be straight forward bugfixes, the only one slightly controversial might be the indentation one: changing tabs to spaces to match Xen coding style. I was unsure whether to include it here, but I think it is best to have it in 4.17 to simplify future (security) backports from master to 4.17, and avoid having to deal with whitespace issues all the time when writing patches. The code here used a style that was different from Xen's, and also different from every other piece of code that I work on, and OCaml indentation tools also only support spaces, not tabs, so there really is no reason to keep the code as is (initially I thought it uses tabs to follow Xen style, but after reading CODING_STYLE I realized that is not true). It is very easy to verify that the patch changes nothing with `git diff -w`, or `git log -p -1`. Edwin Török (15): tools/ocaml/libs/eventchn: do not leak event channels and OCaml 5.0 compat tools/ocaml/libs/xc: OCaml 5.0 compatibility tools/ocaml/libs/{xb, mmap}: use Data_abstract_val wrapper tools/ocaml/xenstored/Makefile: use ocamldep -sort for linking order tools/ocaml/Makefile.rules: do not run ocamldep on distclean tools/ocaml/Makefile.rules: hide -include on *clean CODING_STYLE(tools/ocaml): add 'make format' and remove tabs tools/ocaml/libs/evtchn: add xenevtchn_fdopen bindings tools/ocaml/xenstored/store.ml: fix build error tools/ocaml/xenstored: keep eventchn FD open across live update tools/ocaml/xenstored: do not rebind event channels after live update tools/ocaml/xenstored: log live update issues at warning level tools/ocaml/xenstored: set uncaught exception handler tools/ocaml/xenstored/syslog_stubs.c: avoid potential NULL dereference tools/ocaml/libs/xc: fix use of uninitialized memory in shadow_allocation_get tools/ocaml/Makefile | 5 + tools/ocaml/Makefile.rules | 4 +- tools/ocaml/libs/eventchn/xeneventchn.ml | 11 +- tools/ocaml/libs/eventchn/xeneventchn.mli | 14 +- tools/ocaml/libs/eventchn/xeneventchn_stubs.c | 199 +- tools/ocaml/libs/mmap/mmap_stubs.h | 9 +- tools/ocaml/libs/mmap/xenmmap.ml | 2 +- tools/ocaml/libs/mmap/xenmmap.mli | 4 +- tools/ocaml/libs/mmap/xenmmap_stubs.c | 114 +- tools/ocaml/libs/xb/op.ml | 76 +- tools/ocaml/libs/xb/packet.ml | 30 +- tools/ocaml/libs/xb/partial.ml | 48 +- tools/ocaml/libs/xb/xb.ml | 422 ++-- tools/ocaml/libs/xb/xb.mli | 106 +- tools/ocaml/libs/xb/xenbus_stubs.c | 50 +- tools/ocaml/libs/xb/xs_ring.ml | 28 +- tools/ocaml/libs/xb/xs_ring_stubs.c | 216 +- tools/ocaml/libs/xc/abi-check | 2 +- tools/ocaml/libs/xc/xenctrl.ml | 330 +-- tools/ocaml/libs/xc/xenctrl.mli | 12 +- tools/ocaml/libs/xc/xenctrl_stubs.c | 1428 ++++++------ tools/ocaml/libs/xentoollog/caml_xentoollog.h | 6 +- .../ocaml/libs/xentoollog/xentoollog_stubs.c | 196 +- tools/ocaml/libs/xl/xenlight_stubs.c | 2022 ++++++++--------- tools/ocaml/libs/xs/queueop.ml | 48 +- tools/ocaml/libs/xs/xs.ml | 220 +- tools/ocaml/libs/xs/xs.mli | 46 +- tools/ocaml/libs/xs/xsraw.ml | 300 +-- tools/ocaml/libs/xs/xst.ml | 76 +- tools/ocaml/libs/xs/xst.mli | 20 +- tools/ocaml/test/dmesg.ml | 26 +- tools/ocaml/test/list_domains.ml | 4 +- tools/ocaml/test/raise_exception.ml | 4 +- tools/ocaml/test/xtl.ml | 28 +- tools/ocaml/xenstored/Makefile | 6 +- tools/ocaml/xenstored/config.ml | 156 +- tools/ocaml/xenstored/connection.ml | 594 ++--- tools/ocaml/xenstored/connections.ml | 304 +-- tools/ocaml/xenstored/define.ml | 6 +- tools/ocaml/xenstored/disk.ml | 218 +- tools/ocaml/xenstored/domain.ml | 104 +- tools/ocaml/xenstored/domains.ml | 320 +-- tools/ocaml/xenstored/event.ml | 12 +- tools/ocaml/xenstored/history.ml | 62 +- tools/ocaml/xenstored/logging.ml | 467 ++-- tools/ocaml/xenstored/packet.ml | 20 +- tools/ocaml/xenstored/parse_arg.ml | 106 +- tools/ocaml/xenstored/perms.ml | 216 +- tools/ocaml/xenstored/poll.ml | 68 +- tools/ocaml/xenstored/poll.mli | 4 +- tools/ocaml/xenstored/process.ml | 1212 +++++----- tools/ocaml/xenstored/quota.ml | 74 +- tools/ocaml/xenstored/select_stubs.c | 62 +- tools/ocaml/xenstored/stdext.ml | 190 +- tools/ocaml/xenstored/store.ml | 752 +++--- tools/ocaml/xenstored/symbol.ml | 2 +- tools/ocaml/xenstored/syslog.ml | 48 +- tools/ocaml/xenstored/syslog_stubs.c | 33 +- tools/ocaml/xenstored/systemd_stubs.c | 10 +- tools/ocaml/xenstored/transaction.ml | 352 +-- tools/ocaml/xenstored/trie.ml | 222 +- tools/ocaml/xenstored/trie.mli | 22 +- tools/ocaml/xenstored/utils.ml | 146 +- tools/ocaml/xenstored/xenstored.ml | 1051 ++++----- 64 files changed, 6557 insertions(+), 6388 deletions(-) base-commit: e61a78981364925a43c9cc24dc77b62ff7b93c9f