Hey Raion, here are some points to watch out for after taking a look at your code:
I tend to divide coding into two phases which get repeated iteratively until the code is complete.
Phase no. 1 is getting the main code path done. You will see there's always at least one code path which has to be executed most of the time. This would be your main code path/code line.
Phase no. 2 is adding secondary code paths like error/exception handling, securing code, adjusting style aspects and commenting code.
In abstract:
while (CODE_INCOMPLETE)
{
phase_no1();
phase_no2();
}
Once you get your main line of code working, start completing the code with the different error/exception handling paths. Since you are starting as a developer, don't repeat the same mistakes others already did:
Code without error handling (i.e. code that just works for some cases) is __incomplete__ code. It is tedious to add. Yes. But necessary to get correct code, not just working code (i.e. only main code path).
Code:
fam.cpp:115:
snprintf(msg, sizeof(msg),"%c%d %d %d %s\n", code, reqnum, getuid(), groups[0],
filename);
msg2 = msg + strlen(msg) + 1;
Take above code as an example. You would read through man snprintf and add the corresponding if's or switch's to handle all possible return values.
From IRIX's man snprintf:
if (snprintf() == EXPECTED_NUM)
{
// Main code line
}
else if (... < 0)
{
// Error/exception code line
}
This would be error/exception handling.
And since we are at it: you'll see in the above code that you need the return value from snprintf() to evaluate it in more than one case. At least two approaches can help you here:
ret and if:
1. ret = snprintf();
if (ret == EXPECTED_NUM)
{
}
else if (ret < 0)
{
}
2. switching out
switch (snprintf())
{
case EXPECTED_NUM: ...
default:
// < 0 case
}
Okay, so now the code is handled in all possible paths, which makes it robust, more stable, and complete. What else can be done? Securing it.
Securing code means guaranteeing correct code paths for all possible variable values. The state of your program must always (also: for any case) be deterministic:
ret = snprintf();
if (ret >= 9)
{
// Main code line
}
else
{
// Error/exception code line
}
This code handles all cases assuming groups and filename have been sanitized, validated and secured. Where does the
if (ret >= 9)
come from?
It comes from snprintf()'s format string: "%c%d %d %d %s\n" .
The resulting string, after snprintf(), will include, at least: +1 byte for %c, +3 bytes for all %d, +3 bytes for all spaces, +1 for %s, +1 byte for \n = 1 + 3 + 3 + 1 + 1 = 9.
This is assuming the filename can be relative, so at least one byte must be given. If filename must be absolute, you would add +2, not +1 ('/' + at least one more byte for the filename):
1 + 3 + 3 + 2 + 1 = 10
Ok, what else?
Sanitization vs. validation
Sanitizing refers to the structure of code or values (the real things).
Validation to the architecture of code or values (the logical things).
Sanitizing would be to check for correct characters in the filename string. Validation would mean to check if the given filename is actually valid as input:
if (ret >= 9)
{
// Sanitize filename string
// * Check if correct bytes are given
// Validate filename
// * Check if file exists, is accessible, ...
}
A last few words about coding style. There are many coding styles, but there's one important concept: consistency.
If you have chosen your style, stick to it, always:
Inlined types:
int FAMMonitorFile2(FAMConnection *fc,
fam.cpp:162
Stacked types:
int
FAMMonitorCollection(FAMConnection* fc,
fam.cpp:181
Same for parameters.
Line iffing:
if (userData) FAMStoreUserData(reqnum, userData);
fam.cpp:201
Float iffing:
if (!filename || filename[0] != '/')
return -1;
fam.cpp:90
Wrapped iffing:
if ((reqnum = FAMFindFreeReqnum()) == ILLEGAL_REQUEST) {
return(-1);
}
fam.cpp:161
Spacing and indentation:
return(-1);
fam.cpp:162
return (-1);
fam.cpp:169
Benefits of consistent coding style:
* more readable
* easier to maintain
As an example, if you try grepping a function name and you mixed both function declaration/definition types like above (inlined and stacked) you would probably need to grep twice or get much more output:
grep FuncName * # Lots of output
grep '^FuncName' * # Only catches FuncName if stacked
grep '^int FuncName' * # Only catches FuncName if inlined
Some other remarkable points:
Code:
while (*c != '\0') {
fe->filename[i++] = *c++;
}
last = fe->filename[i-1];
fe->filename[i-1] = '\0';
fam.c:293
Just to verify this is correct. You copy a portion of a string, c, to filename and then set its (filename) last character to \0?
Code:
// A Necessary Global
static char* userEndExistArray;
fam.cpp:49
static int* userDataIndexArray;
void FAMInit()
{
static int firstTime = 1;
// allocate arrays
if (firstTime) {
userDataArray = (void**) malloc(maxUserData * sizeof(void*));
userDataIndexArray = (int*) malloc(maxUserData * sizeof(int*));
userEndExistArray = (char*) malloc(maxUserData * sizeof(char*));
for (int i=0; i<maxUserData; i++) {
userDataIndexArray[i] = -1;
}
firstTime = 0;
}
helpers.cpp:25
Should be:
Code:
userDataIndexArray = (int*) malloc(maxUserData * sizeof(int));
userEndExistArray = (char*) malloc(maxUserData * sizeof(char));
(12-29-2021, 02:42 AM)Raion Wrote: As I speak I was successful in reconstruction of it from the documentation he provided. He provided a significant analysis of each and I did my best to match the CFront era C++ function design and setup. As a result the code quality is atrocious and likely will not be my finest work, now or ever. I am currently working in getting it compiling and will build a few programs for testing purposes.
If you did all this code by yourself then I'm impressed. It's not what I would expect from someone without system programming experience. This is huge, Raion.
Cheers
NEED -> 