|
From: Andras E. <and...@in...> - 2004-02-26 12:56:58
|
is there any reason to list individuals beside the list address? should i do it, too? On Thu, 26 Feb 2004 12:21:52 +0100 (MET), Szakacsits Szabolcs = <sz...@si...> wrote: >> - /* Need two bytes for null terminator. */ >> - maxlen -=3D 2; >> + /* Convert maxlen from bytes to unicode characters. */ >> + maxlen /=3D sizeof(uchar_t); >> + /* Need space for null terminator. */ >> + maxlen--; >This is still wrong. If maxlen was <=3D 1 and src was bigger, then we = have a >buffer overflow (currently it never should occure as Anton also pointed >out). Only Andras 3rd solution was correct. the options: - an if() - an assert() - a comment i assumed the if() was intentionally left out (efficiency :O), so i tried the other two approaches; as you don't seem to like asserts(), do you think the comment * @par constraint: @dst must be able to hold at least the * terminating null [@size >=3D sizeof( uchar_t )] was enough ? >Personally I prefer when by reading the code, it documents itself = without >explicit /* */ or // comments. Real comments are for non-trivial things, >things that's thought but not in the code, function description, = FIXME's, >etc. i don't know what kind of audience are you targeting with this code; and you may be right, not using ((almost) trivial) comments=20 may actually be helpful as it raises the entry treshold and can prevent well-meaning newbies from poking around with the code and hurting themselves >Please note, the coding style is the > > ntfsprogs/doc/CodingStyle > kernel/Documentation/CodingStyle i've checked out the latter (it was referenced in the readme or somewhere), now i'll read the former, too :O) >Other reasons are=20 > > - if the code moved to the library then it would be pretty rude > to abort instead giving the chance for the caller to clean up but then it would be pretty rude to leave it as it is (without the assert) and let it abort with a involuntary core dump instead :O) > - important info on the failure context can get lost (e.g. to=20 > debug user problems) assert() is not for production code (users) -- they'll just get the core dump :O) but ok, ok, got the point, no assert()s, it's fine by me br, andras |
