2
\$\begingroup\$

I am trying to make a script that compares 2 .txt files.

It works, but I wanted to double-check if there's something I missed about these system calls (since I'm new at this). Is there any improvement you would've done or anything else you think can help me improve with it (especially with this mmap thing)?

The script returns 2 if it's the same file and 1 otherwise.

#include<sys/stat.h>
#include<unistd.h>
#include<stdio.h>
#include<stdlib.h>
#include <sys/mman.h>
#include <sys/types.h>
#include <sys/stat.h>
#include <fcntl.h>

int main(int argc , char * argv[])
{
    if(argc != 3)
    {
        printf("something wrong with variables\n");
        exit(-1);
    }

    char* filename1 = argv[1];
    char* filename2 = argv[2];
    char* addr1;
    char* addr2;
    struct stat stat_p1;
    struct stat stat_p2;
    int fileSize1=0,fileSize2=0;

    if(stat(filename1,&stat_p1) == -1)
    {
        printf("error occurred while attempting to stat %s\n" , filename1);
        exit(-2);
    }

    if(stat(filename2,&stat_p2) == -1)
    {
        printf("error occurred while attempting to stat %s\n" , filename2);
        exit(-2);
    }

    if((fileSize1=stat_p1.st_size) != (fileSize2=stat_p2.st_size)) //checks if the size is different than its not the same file.
    {
        return 1;
    }
    else
    {
        int fd1,fd2;
        if((fd1 = open(filename1 ,O_RDONLY)) < 0)
        {
            printf("error opening file %s\n",filename1);
            exit(-3);
        }
        if((fd2 = open(filename2 ,O_RDONLY)) < 0)
        {
            printf("error opening file %s\n",filename2);
            exit(-3);
        }

        addr1 = mmap(NULL, fileSize1+ 1, PROT_READ,MAP_PRIVATE, fd1, 0);
        if (addr1 == MAP_FAILED)
        {
            printf("mmap failed\n");
            exit(-4);
        }

        addr2 = mmap(NULL, fileSize2+ 1, PROT_READ,MAP_PRIVATE, fd2, 0);
        if (addr2 == MAP_FAILED)
        {
            printf("mmap failed\n");
            exit(-4);
        }
        for(int i=0;i<fileSize1+1;i++)
        {
            if(addr1[i] != addr2[i])
            {
                return 1;
            }
        }
        close(fd1);
        close(fd2);
    }
    return 2;
}

(It's in the main file because it's meant to be used as part of testing program later.)

\$\endgroup\$
1
  • 1
    \$\begingroup\$ Use memcmp(addr1, addr2, filesize1) instead of rolling your own loop. Also, why the + 1 to the file sizes? \$\endgroup\$
    – G. Sliepen
    Apr 2 '20 at 18:36
2
\$\begingroup\$

Only for the pedantic

With non-2's complement, the below will compare +0 as matching -0.

char* addr1;
char* addr2;
...
if(addr1[i] != addr2[i]) ... // Oops. +0 == -0

Instead use unsigned char *

unsigned char* addr1;
unsigned char* addr2;
...
if (addr1[i] != addr2[i]) ... // OK.  Only one kind of 0

Better yet, use memcmp().

Return values

"... returns 2 if it's the same file and 1 otherwise." --> Not quite.

Code also returns via exit() with other values.

exit(-1);
exit(-3);
exit(-4);

+ 1??

Off by 1. No need for + 1. G. Sliepen.

// for(int i=0;i<fileSize1+1;i++)
for (int i=0; i<fileSize1; i++)

Matching type

.st_size is typed as off_t. I'd expect fileSize1, fileSize2, i as the same.

off_t fileSize1;
...
for (off_t i = 0; i < fileSize1; i++)

Big files

.st_size is a off_t, some signed type. mmap(void *addr, size_t length, ...) takes a length of size_t, some unsigned type.

To do this right, code may need to call mmap() multiple times as file size can exceed SIZE_MAX.

Something like:

#define CHUNK  (‭1048576‬ /* 1 Meg */)

off_t fileSize1;
fileSize1 = stat_p1.st_size;

....

for (off_t offset = 0; offset < fileSize1; offset += CHUNK) {
  size_t map_size1 = CHUNK;
  if (fileSize1 - offset < CHUNK) {
    map_size1 = (size_t) (fileSize1 - offset);
  }

  addr1 = mmap(NULL, map_size1, PROT_READ,MAP_PRIVATE, fd1, offset);

  // as above for map_size1, addr2

  for (size_t i = 0; i < map_size1; i++)
    ...
  // or
  if (memcmp(addr1, addr2, map_size1) .... 
\$\endgroup\$
1
\$\begingroup\$

Here are some things that may help you improve your program.

Use standard library calls where appropriate

Instead of your for loop, I would recommend using memcmp() to accomplish the same thing, but likely more efficiently since the library version typically compares more than one byte at a time.

Define variables where they are declared

The addr1 and addr2 variables are only used within the else clause, so instead of having them at the top (and uninitialized), it's probably better to declare and initialize the variable at the same time:

char *addr1 = mmap(NULL, fileSize1+ 1, PROT_READ,MAP_PRIVATE, fd1, 0);

Eliminate "magic numbers"

Things like the 0 in the code line quoted above are OK, because programmers reading this will know what that code is doing. However, when you write exit(-3); it's not at all obvious whether -3 is the correct value, or exactly what it represents. Better would be to use a named const value there such as const int ERROR_FILE_OPEN = -3;

Consider explicitly releasing all resources

It's true that an mmap will automatically be released when the program terminates, but you may wish to explicitly call mmunmap explicitly before closiing the files. Note that just closing the files does not release the memory.

Be aware of possible race conditions

It's admittedly unlikely, but it's possible that a file could be modified between your call to stat and the open call. If it were, for example, a log file, the actual file would be longer than the mmap region. You may find it an acceptable risk, but it's worth knowing about this potential problem.

\$\endgroup\$
1
  • \$\begingroup\$ thanks ! ill go over the things you mentioned \$\endgroup\$ Apr 2 '20 at 18:08
1
\$\begingroup\$

A few things I noticed:

  • You have a double include of sys/stat.h
  • When checking through value of argc, instead of saying “Something wrong with variables”, it would be better to print a notice that the user gave the wrong number of arguments, along with a small usage statement. There’s also an error code for that case, I believe.
  • You set the file size variables as an int, but I would bet that st_size returns a size_t or at least an unsigned value, I’d suggest checking the man page and changing that declaration
  • In the conditional branch where the file sizes are different is followed, it might be nice to print a message saying that the file sizes are different
  • Many of the functions you call( stat, fopen,etc...) set errno on failure. It might be nice if when you handle those errors, you printed out the error with strerror(errno) so the user has a better understanding of the error (make sure to include errno.h)
  • At the end of your program, it is assumed the files are equal, change return 2 to return EXIT_SUCCESS
  • None of these are fatal, but worth considering for quality.

So now that I have torn apart your code: It looks good, the formatting was clear for me to read. I couldn’t detect anything concerning other than what was listed. The best thing I can think to do moving on would be to run valgrind on it, run through it with a debugger and do a bunch of tests.

\$\endgroup\$
3
  • \$\begingroup\$ sure ill go over it , thank you very much ! \$\endgroup\$ Apr 2 '20 at 18:09
  • \$\begingroup\$ .st_size is off_t. Ref. It is signed. ref \$\endgroup\$ Apr 4 '20 at 22:15
  • \$\begingroup\$ Thank you! I don’t use stat often enough to recall by memory. \$\endgroup\$
    – Jordan
    Apr 4 '20 at 22:18

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.