code review - graceful service monitor

This is a discussion on code review - graceful service monitor within the C Programming forums, part of the General Programming Boards category; hi guys, any chance of reviewing this code - its not very big. i origanlly wrote this in java - ...

  1. #1
    Registered User
    Join Date
    Aug 2002
    Posts
    351

    code review - graceful service monitor

    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
    Attached Files Attached Files

  2. #2
    Registered User
    Join Date
    Aug 2002
    Posts
    351
    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?
    Last edited by rotis23; 12-05-2002 at 05:11 AM.

  3. #3
    Me want cookie! Monster's Avatar
    Join Date
    Dec 2001
    Posts
    680
    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);

  4. #4
    Registered User
    Join Date
    Aug 2002
    Posts
    351
    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.

  5. #5
    End Of Line Hammer's Avatar
    Join Date
    Apr 2002
    Posts
    6,231
    >>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.
    When all else fails, read the instructions.
    If you're posting code, use code tags: [code] /* insert code here */ [/code]

  6. #6
    Registered User
    Join Date
    Aug 2002
    Posts
    351
    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.
    Last edited by rotis23; 12-09-2002 at 10:23 AM.

  7. #7
    Me want cookie! Monster's Avatar
    Join Date
    Dec 2001
    Posts
    680
    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))

  8. #8
    End Of Line Hammer's Avatar
    Join Date
    Apr 2002
    Posts
    6,231
    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.
    When all else fails, read the instructions.
    If you're posting code, use code tags: [code] /* insert code here */ [/code]

  9. #9
    Registered User
    Join Date
    Aug 2002
    Posts
    351
    changed to strdup!

    if anyone's interested in the updated code, i'll post it. maybe useful for some.
    Last edited by rotis23; 12-10-2002 at 05:27 AM.

Popular pages Recent additions subscribe to a feed

Similar Threads

  1. Replies: 10
    Last Post: 05-24-2009, 01:55 AM
  2. code review...
    By dar32 in forum C Programming
    Replies: 10
    Last Post: 10-26-2008, 06:53 AM
  3. running my program as service
    By daher in forum Windows Programming
    Replies: 5
    Last Post: 09-05-2008, 01:30 PM
  4. Code review
    By Elysia in forum C++ Programming
    Replies: 71
    Last Post: 05-13-2008, 10:42 PM
  5. Obfuscated Code Contest
    By Stack Overflow in forum Contests Board
    Replies: 51
    Last Post: 01-21-2005, 04:17 PM

1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21