In article <20220614080613.b4570f...@cvs.netbsd.org>, Robert Elz <source-changes-d@NetBSD.org> wrote: >-=-=-=-=-=- > >Module Name: src >Committed By: kre >Date: Tue Jun 14 08:06:13 UTC 2022 > >Modified Files: > src/sbin/raidctl: rf_configure.c > >Log Message: >Fix some config file parsing. > >First, and what got me started on this set of cleanups, the queue >length in the "queue" section (START queue) is limited to what will >fit in a char without losing accuracy (I tried setting it to 200, >rather than the more common (universal?) 100 and found that the >value configured into the array was -56 instead. > >Why the value needs to be passed through a char variable I have no >idea (it is an int in the filesystem raidframe headers) - but that's >the way it is done, and changing it would be an ABI change I believe >(and so need versioning to alter) and that isn't worth it for this >(or not now, IMO). > >Instead check that the value in the char is the same value as was >read from the config file, and complain if not. Those of you with >unsigned chars will be able to have queue lengths up to 255, the >rest of us are limited to 127. > >While looking at that, I noticed some code that obviously fails to >understand that scanf("%s") will never return a string containing >spaces, and proceeded to attempt to remove trailing spaces from the >result ... amusingly, after having used the result for its intended >purpose (non existent trailing spaces unremoved), after which that >buffer was never used again. That code is now gone (but for now, >just #if 0'd rather than actually deleted - it should be cleaned up >sometime). > >Then I saw some other issues with how the config was parsed - a >simple (unbounded) scanf("%s") into a buffer, which hypothetically >might not be large enough (not a security issue really, raidctl has >no special privs, and it isn't likely that root could easily be >tricked into running it on a bogus config file - or not without >looking first anyway, and a huge long string would rather stand >out). Bound the string length to something reasonable, and >assert() that the buffer is big enough to contain it. > >Lastly, in the event of one particular detected error in the >config file, the code would write a warning, but then just go >ahead and use the bad data (or nothing perhaps) anyway - a >failure of logic flow (unlikely to have ever happened, everyone >seems to simply copy the sample config from the man page, and >make minor adjustments as needed). > >If any of these changes make any difference to anyone (except >me with my attempt to make longer queues - for no particularly >well thought out reason), I'd be very surprised.
There is really no type consistency raidframevar.h: char maxOutstandingDiskReqs; /* # concurrent reqs to be sent to a raidframevar.h: int maxOutstanding; /* maxOutstanding disk requests */ rf_diskqueue.h: long maxOutstanding; /* max # of I/Os that can be outstanding on a rf_raid.h: int maxOutstanding; Nevertheless why don't we make the char one unsigned char? It will not be an ABI change and it will make it 255 for everyone. christos