Skip to main content
Code Review

Return to Answer

added 950 characters in body
Source Link
syb0rg
  • 21.9k
  • 10
  • 113
  • 192
  • Overall, you are doing a lot of unnecessary work. You could just use getifaddrs() to get the IP address, MAC address, and interface name. There are plenty of code examples online on how you can use it to obtain all of that information, including an example in the function documentation itself.

    You could also read directly from the file /sys/class/net/eth0/address for the MAC address (but that isn't very portable).

  • strtok() is limited to tokenizing only one string (with one set of delimiters) at a time, and it can't be used while threading. Therefore, strtok() is considered deprecated.

    Instead, use strtok_r() or strtok_s() which are threading-friendly versions of strtok(). The POSIX standard provided strtok_r(), and the C11 standard provides strtok_s(). The use of either is a little awkward, because the first call is different from the subsequent calls.

    1. The first time you call the function, send in the string to be parsed as the first argument.

    2. On subsequent calls, send in NULL as the first argument.

    3. The last argument is the scratch string. You don't have to initialize it on first use; on subsequent uses it will hold the string as it is parsed so far.

    To demonstrate its use, I've written a simple line counter (of only non-blank lines) using the POSIX standard one. I'll leave the choice of what version to use and implementation into your program up to you.

     #include <string.h> // strtok_r
     int countLines(char* instring)
     {
     size_t counter = 0;
     char *scratch, *txt;
     char *delimiter = "\n";
     for (; (txt = strtok_r((!counter ? instring : NULL), delimiter, &scratch)); counter++);
     return counter;
     }
    
  • fopen(), a widely-used file I/O functions that you are using, got a facelift in C11. It now supports a new exclusive create-and-open mode ("...x"). The new mode behaves like O_CREAT|O_EXCL in POSIX and is commonly used for lock files. The "...x" family of modes includes the following options:

  • wx create text file for writing with exclusive access.

  • wbx create binary file for writing with exclusive access.

  • w+x create text file for update with exclusive access.

  • w+bx or wb+x create binary file for update with exclusive access.

Opening a file with any of the exclusive modes above fails if the file already exists or cannot be created. Otherwise, the file is created with exclusive (non-shared) access. Additionally, a safer version of fopen() called fopen_s() is also available. That is what I would use in your code if I were you, but I'll leave that up for you to decide and change.

  • You aren't abiding by common variable naming practices for C by using PascalCase. Most people either write in camelCase or in snake_case.

  • You probably shouldn't use the read() function anymore; instead prefer fread().

  • Overall, you are doing a lot of unnecessary work. You could just use getifaddrs() to get the IP address, MAC address, and interface name. There are plenty of code examples online on how you can use it to obtain all of that information, including an example in the function documentation itself.

    You could also read directly from the file /sys/class/net/eth0/address for the MAC address (but that isn't very portable).

  • strtok() is limited to tokenizing only one string (with one set of delimiters) at a time, and it can't be used while threading. Therefore, strtok() is considered deprecated.

    Instead, use strtok_r() or strtok_s() which are threading-friendly versions of strtok(). The POSIX standard provided strtok_r(), and the C11 standard provides strtok_s(). The use of either is a little awkward, because the first call is different from the subsequent calls.

    1. The first time you call the function, send in the string to be parsed as the first argument.

    2. On subsequent calls, send in NULL as the first argument.

    3. The last argument is the scratch string. You don't have to initialize it on first use; on subsequent uses it will hold the string as it is parsed so far.

    To demonstrate its use, I've written a simple line counter (of only non-blank lines) using the POSIX standard one. I'll leave the choice of what version to use and implementation into your program up to you.

     #include <string.h> // strtok_r
     int countLines(char* instring)
     {
     size_t counter = 0;
     char *scratch, *txt;
     char *delimiter = "\n";
     for (; (txt = strtok_r((!counter ? instring : NULL), delimiter, &scratch)); counter++);
     return counter;
     }
    
  • You aren't abiding by common variable naming practices for C by using PascalCase. Most people either write in camelCase or in snake_case.

  • You probably shouldn't use the read() function anymore; instead prefer fread().

  • Overall, you are doing a lot of unnecessary work. You could just use getifaddrs() to get the IP address, MAC address, and interface name. There are plenty of code examples online on how you can use it to obtain all of that information, including an example in the function documentation itself.

    You could also read directly from the file /sys/class/net/eth0/address for the MAC address (but that isn't very portable).

  • strtok() is limited to tokenizing only one string (with one set of delimiters) at a time, and it can't be used while threading. Therefore, strtok() is considered deprecated.

    Instead, use strtok_r() or strtok_s() which are threading-friendly versions of strtok(). The POSIX standard provided strtok_r(), and the C11 standard provides strtok_s(). The use of either is a little awkward, because the first call is different from the subsequent calls.

    1. The first time you call the function, send in the string to be parsed as the first argument.

    2. On subsequent calls, send in NULL as the first argument.

    3. The last argument is the scratch string. You don't have to initialize it on first use; on subsequent uses it will hold the string as it is parsed so far.

    To demonstrate its use, I've written a simple line counter (of only non-blank lines) using the POSIX standard one. I'll leave the choice of what version to use and implementation into your program up to you.

     #include <string.h> // strtok_r
     int countLines(char* instring)
     {
     size_t counter = 0;
     char *scratch, *txt;
     char *delimiter = "\n";
     for (; (txt = strtok_r((!counter ? instring : NULL), delimiter, &scratch)); counter++);
     return counter;
     }
    
  • fopen(), a widely-used file I/O functions that you are using, got a facelift in C11. It now supports a new exclusive create-and-open mode ("...x"). The new mode behaves like O_CREAT|O_EXCL in POSIX and is commonly used for lock files. The "...x" family of modes includes the following options:

  • wx create text file for writing with exclusive access.

  • wbx create binary file for writing with exclusive access.

  • w+x create text file for update with exclusive access.

  • w+bx or wb+x create binary file for update with exclusive access.

Opening a file with any of the exclusive modes above fails if the file already exists or cannot be created. Otherwise, the file is created with exclusive (non-shared) access. Additionally, a safer version of fopen() called fopen_s() is also available. That is what I would use in your code if I were you, but I'll leave that up for you to decide and change.

  • You aren't abiding by common variable naming practices for C by using PascalCase. Most people either write in camelCase or in snake_case.

  • You probably shouldn't use the read() function anymore; instead prefer fread().

Source Link
syb0rg
  • 21.9k
  • 10
  • 113
  • 192

A few notes:

  • Overall, you are doing a lot of unnecessary work. You could just use getifaddrs() to get the IP address, MAC address, and interface name. There are plenty of code examples online on how you can use it to obtain all of that information, including an example in the function documentation itself.

    You could also read directly from the file /sys/class/net/eth0/address for the MAC address (but that isn't very portable).

  • strtok() is limited to tokenizing only one string (with one set of delimiters) at a time, and it can't be used while threading. Therefore, strtok() is considered deprecated.

    Instead, use strtok_r() or strtok_s() which are threading-friendly versions of strtok(). The POSIX standard provided strtok_r(), and the C11 standard provides strtok_s(). The use of either is a little awkward, because the first call is different from the subsequent calls.

    1. The first time you call the function, send in the string to be parsed as the first argument.

    2. On subsequent calls, send in NULL as the first argument.

    3. The last argument is the scratch string. You don't have to initialize it on first use; on subsequent uses it will hold the string as it is parsed so far.

    To demonstrate its use, I've written a simple line counter (of only non-blank lines) using the POSIX standard one. I'll leave the choice of what version to use and implementation into your program up to you.

     #include <string.h> // strtok_r
     int countLines(char* instring)
     {
     size_t counter = 0;
     char *scratch, *txt;
     char *delimiter = "\n";
     for (; (txt = strtok_r((!counter ? instring : NULL), delimiter, &scratch)); counter++);
     return counter;
     }
    
  • You aren't abiding by common variable naming practices for C by using PascalCase. Most people either write in camelCase or in snake_case.

  • You probably shouldn't use the read() function anymore; instead prefer fread().

C99 & C11 §7.19

Many implementations of the C runtime environment, most notably the UNIX operating system, provide, aside from the standard I/O library’s fopen(), fclose(), fread(), fwrite(), and fseek(), a set of unbuffered I/O services, open(), close(), read(), write(), and lseek(). The C89 Committee decided not to standardize the latter set of functions.

In addition, buffered I/O is always faster than unbuffered. There are cases where you might want to use unbuffered, such as whenever you want to ensure that the output has been written before continuing. But in this case you will want to use fread().

  • Declare your function parameters as void if you don't take in any arguments.

     int main(void)
    
  • Initialize variables upon declaration when you can.

lang-c

AltStyle によって変換されたページ (->オリジナル) /