On 21.02.24 08:26, Peter Eisentraut wrote: > On 14.02.24 17:52, Peter Eisentraut wrote: >> A gentler way might be to start using some perlcritic policies like >> InputOutput::RequireCheckedOpen or the more general >> InputOutput::RequireCheckedSyscalls and add explicit error checking at >> the sites it points out. > > Here is a start for that. I added the required stanza to perlcriticrc > and started with an explicit list of functions to check: > > functions = chmod flock open read rename seek symlink system > > and fixed all the issues it pointed out. > > I picked those functions because most existing code already checked > those, so the omissions are probably unintended, or in some cases also > because I thought it would be important for test correctness (e.g., some > tests using chmod). > > I didn't design any beautiful error messages, mostly just used "or die > $!", which mostly matches existing code, and also this is > developer-level code, so having the system error plus source code > reference should be ok. > > In the second patch, I changed the perlcriticrc stanza to use an > exclusion list instead of an explicit inclusion list. That way, you can > see what we are currently *not* checking. I'm undecided which way > around is better, and exactly what functions we should be checking. (Of > course, in principle, all of them, but since this is test and build > support code, not production code, there are probably some reasonable > compromises to be made.)
After some pondering, I figured the exclude list is better. So here is a squashed patch, also with a complete commit message.
Btw., do we check perlcritic in an automated way, like on the buildfarm?