code review

**rotis23** · 12-05-2002

hi guys,

any chance of reviewing this code - its not very big.

i origanlly wrote this in java - but wrote a c version becuase that one would hang - i presumed some jvm problem. now i have the same with this.

any suggestions - the hang occurs every few thousand calls - from another c program using popen.

does this suggest a problem with the socket libraries etc? its very hard to debug because of when i occurs, maybe someone can find an error in the code.

TIA, rotis23

linux red hat 7.2
gcc 2.96
program function - check a remote service is funtioning by gracefully connecting and disconnecting.
compile line - gcc -D_GNU_SOURCE -Wall portprobe.c -o portprobe

**rotis23** · 12-05-2002

ok, i've noticed an error in the code when checking the socket creation.

should read:

Code:

s = socket(PF_INET,SOCK_STREAM,0);
        if(s == -1)
                return "- - Could not create socket.";

oops! i'll reply if the problem persists.

any other comments?

**Monster** · 12-05-2002

Found nothing stange in your code. Maybe the arguments in main:

Code:

if(argv[3] != NULL)
   timeoutin = atoi(argv[3]);
else
   timeoutin = 0;

result = probe(argv[1],argv[2],timeoutin);

I'm not sure about this but if arg = 1 (no arguments) then argv[1] and above are undefined. You should use the argc argument to check the number of arguments:

Code:

int timeoutin = 0;

if(argv > 3)
   timeoutin = atoi(argv[3]);

result = probe(argc > 1 ? argv[1] : NULL, argc > 2 ? argv[2] : NULL);

Tip (not an error):

Code:

result = malloc(sizeof(char) * 100);
//result = strtok(rxbuf,"\n");
strcpy(result,rxbuf);

Can be replaced by:

Code:

result = strdup(rxbuf);

**rotis23** · 12-05-2002

ok thanks,

i'm not really worried about the input because i control the call.

strdup seems cool - i presume its safer, it terms of getting lengths right.

**Hammer** · 12-05-2002

>>if arg = 1 (no arguments) then argv[1] and above are undefined.
Not quite, argv[1] (or rather argv[argc]) is guaranteed to be a NULL pointer.

[edit]And if you're going to use strdup(), remember you are responsible for free()'ing the memory it creates.

**rotis23** · 12-09-2002

thanks for your comments guys.

you're right about returning the static strings salem and about not closing when an error occurs.

after reading about fdopen (man page), fclose will close the stream and the file descriptor (in this case the socket).

so, use close if stream has not been opened - i.e. an error occurs.

but use fclose when both have been opened.

**Monster** · 12-09-2002

Originally posted by Hammer
>>if arg = 1 (no arguments) then argv[1] and above are undefined.
Not quite, argv[1] (or rather argv[argc]) is guaranteed to be a NULL pointer.

[edit]And if you're going to use strdup(), remember you are responsible for free()'ing the memory it creates.

OK, learned something new here, but that still doesn't cover the 'above' part (if(argv[3] != NULL))

**Hammer** · 12-09-2002

Originally posted by Monster
... but that still doesn't cover the 'above' part (if(argv[3] != NULL))

True. I didn't look at the complete code, and now I have, I can see what you mean.

Code:

result = malloc(sizeof(char) * 100);
//result = strtok(rxbuf,"\n");
strcpy(result,rxbuf);

That's a bit nasty. If you know how much dynamic memory you need, why not malloc the exact amount instead of guessing and leaving the buffer open to exploitation.

**rotis23** · 12-10-2002

changed to strdup!

if anyone's interested in the updated code, i'll post it. maybe useful for some.

Thread: code review - graceful service monitor

Thread Tools

Search Thread

Display

code review - graceful service monitor

Similar Threads

Code review for a class that spawns a new process and allows IPC

code review...

running my program as service

Obfuscated Code Contest