views:

225

answers:

3

I have been trying to debug my code whenever I had free-time for the past day and a half and I don't know what is wrong with my code. When I add the close() function to a recursive call, the program gives me an invalid pointer. But when I remove the close() function call the program runs fine, except it does not do what it is supposed to do, which is:

  • add up all the file sizes in a user input directory
  • open sub-directories, if any, and add up all the files inside the sub-directory

Instead, it adds up all the file sizes in the input directory and is able to open the last sub-directory and add the files within that directory to the total file size count.

I am trying to do this with threads. The main() function creates one main thread from the user input directory and runs opendirectory() off the bat.

/*
 * Iterates through given directory
 */
void *opendirectory(void *t)
{
 pthread_mutex_lock(&dirlock);
 DIR *dpntr;
 struct dirent *dentry;
 char new_directory[512], dir = t;

 printf("OPENING DIRECTORY ... %s\n", t);

 /* Checks if given directory can be opened */
 if((dpntr = opendir(t)) == NULL) {
  printf("DIRECTORY FAILED ...%s\n",t);
  perror("ERROR -- COULD NOT OPEN DIR");
  pthread_exit(NULL);
 }

 printf("DIRECTORY OPENED: %s\n", t);

 /* Read each file in current directory */
 while ((dentry = readdir(dpntr)) != NULL ) {
  /* Ignore special directories */
  if(strcmp(dentry -> d_name, ".") == 0 || strcmp(dentry -> d_name, "..") == 0) {
   continue;
  } else {
   compilelist( t, dentry->d_name );
  }
 }

 pthread_mutex_unlock(&dirlock);
 /* Checks if directory can be closed */
 if(closedir(dpntr) < 0)
  printf("ERROR CLOSING %s.\n", t);

}

This is the function that will determine if a new thread should be created and is supposed to run recursively.

/*
 * Determines if current file is a directory
 * Creates a new thread if true
 */
void compilelist (const char* dirname, const char *filename)
{
    pthread_mutex_lock(&filelock);
    struct stat statdata;
    char *filepathname, *dpntr;

    /* Allocate memory for filepathname */
    if((filepathname = (char *) malloc(sizeof(char) * strlen(dirname))) == NULL)
    {
     printf("CANNOT ALLOCATE MEMORY FOR FILE PATH NAME.");
     pthread_exit(NULL);
    }

    /* Concats directory name with file name */
    if(dirname[strlen(dirname) -1] == '/')
    {
     pthread_mutex_lock(&pathlock);
     sprintf(filepathname, "%s%s", dirname, filename);
     pthread_mutex_unlock(&pathlock);
    }else
    {
     pthread_mutex_lock(&pathlock);
     sprintf(filepathname, "%s/%s", dirname, filename);
     pthread_mutex_unlock(&pathlock);
    }

    lstat(filepathname, &statdata);

    /* Calls print_statdata() if current item is a file */
    if(!(S_ISDIR(statdata.st_mode)))
    {
     printf("FILE: %s\n", filepathname);
     if(!stat( filepathname, &statdata))
     {
      print_statdata( filename, &statdata );
     }
     else {
      fprintf (stderr, "GETTING STAT FOR %s", filepathname);
      perror( "ERROR IN STATDATA WHILE GETTING STAT");
     }
    }
    /* Recursive call to opendirectory() */
    else {
     pthread_mutex_lock(&dircountlock);
     dirCount++;
     pthread_mutex_unlock(&dircountlock);
     dpntr = filepathname;
     free(filepathname);
     printf("SUB-DIRECTORY THREAD: %s\nTHREAD ID NUMBER: %d\n", dpntr, dirCount);
     pthread_attr_setdetachstate(&attr, PTHREAD_CREATE_JOINABLE);
     pthread_create(&threads[dirCount-1], &attr, opendirectory, (void *)dpntr);
    }

    pthread_mutex_unlock(&filelock);

}

Here is the main()

/*
 * Main function prompts user for a directory
 */
int main(int argc, char *argv[])
{
    int i;
    char *dPtr;
    // pthread_attr_t attr;

    printf("ENTER A DIRECTORY:\n\t");
    scanf("%s", directory);
    dPtr = directory;

    /* Initialize mutex and condition variable objects */
    pthread_mutex_init(&mutex, NULL);
    pthread_mutex_init(&filelock, NULL);
    pthread_mutex_init(&dirlock, NULL);
    pthread_mutex_init(&dircountlock, NULL);
    pthread_cond_init (&count_threshold_cv, NULL);

    /* For portability, explicitly create threads in a joinable state */
    pthread_attr_init(&attr);
    pthread_attr_setdetachstate(&attr, PTHREAD_CREATE_JOINABLE);
    pthread_create(&threads[0], &attr, opendirectory, (void *)dPtr);

    /* Wait for all threads to complete */
    for (i = 0; i < dirCount; i++) {
     pthread_join(threads[i], NULL);
    }

    printf("TOTAL DIRECTORY SIZE: %d\n", dirSize);

    /* Clean up and exit */
    pthread_attr_destroy(&attr);
    pthread_mutex_destroy(&mutex);
    pthread_mutex_destroy(&filelock);
    pthread_mutex_destroy(&dirlock);
    pthread_mutex_destroy(&dircountlock);
    pthread_cond_destroy(&count_threshold_cv);
    pthread_exit (NULL);

}

And the global variables ...

pthread_mutex_t mutex;
pthread_mutex_t dirlock;
pthread_mutex_t filelock;
pthread_mutex_t dircountlock;
pthread_mutex_t threadlock;
pthread_cond_t count_threshold_cv;
pthread_attr_t attr;
pthread_t threads[128]; // handles up to 128 threads (i.e. 128 directories, change accordingly)
char directory[512];
int dirSize = 0;
int dirCount = 1; // user's input directory

I feel that the pthread_create() called at the bottom of the compilelist() function is not working properly. The threads[] refers to a global array of threads that has a default size of 20, assuming that there will be no more than 20 total directories. dirCount starts off at 1 because of the user's input directory and increases as new directories are encountered.

A: 

First problem:

whenever I had free-time for the past day and a half

Don't do that, your brain isn't built for it. Allocate a time, tell your workmates/wife-and-kids that, if they bother you during this time, there will be gunshots and police involvement :-)

Other problems: no idea (hence the community wiki).

paxdiablo
haha ... it's a busy time of month :)
Kenji
One thing to watch out for, @Kenji (I don't think this is your *specific* problem). I can't see, with a cursory glance, anything that prevents the main thread from changing filepathname before your child thread has a chance to use it (via the dPtr pointer).
paxdiablo
I think you may be on to something, I am trying to switch some global variables to local and going to recompile ...
Kenji
@Kenji, what I tend to do in that case is have a mutex-protected flag variable. Main thread sets it to 1 before creating child thread then loops until it's 0. Child thread copies relevant data (data, not pointers to data) to local storage then sets it to 0 and carries on. That guarantees each thread is not stepping on the other.
paxdiablo
I will look into that and edit the code ...
Kenji
+2  A: 

Your code:

dpntr = opendir(t)

...

if(closedir(t) < 0)

should be:

if(closedir(dpntr) < 0)
wrang-wrang
OH ... thanks ... that was a simple mistake that I seriously overlooked, should have triple checked that!
Kenji
make sure to mark this as the answer then
Earlz
I have changed it, but I am still getting a segmentation fault when I run the program on my laptop. Can't seem to connect to a Linux system by ssh'ing at the moment to test ...
Kenji
+1  A: 

Here I found 2 problems of your code:

  1. As wrang-wrang metioned, closedir(t) leads segfault.

  2. "char filepathname[512];" of compilelist() is a local memory buffer, but you pass it to your thread (opendirectory) and use it continuously. You should use copying or dynamic-allocation instead.

Effo Upd@2009nov17: After fixing above 2 points, it works fine on my FC9 x86_64 so far. Btw: threads number 20 is really not enough.

EffoStaff Effo
Will edit post right now for closedir(), forgot to update that in last edit. I will definitely check the char filepathname[512] usage and consider how to go about it. Thanks.
Kenji
Okay, so I believe I have set it up correctly to use dynamic allocation instead, but I am still getting errors on threads not opening up sub-directories. I will post my new code in a bit.
Kenji
Am I not supposed to free(filepathname)? I thought I had to do so in order to malloc correctly for recursive calls ...
Kenji
1. free it in the thread opendirectory(). 2. if point 1 then do dynamic-alloc in main too. 3. don't forget to free it on other branches such as !ISDIR() and opendir() failure, and so on.
EffoStaff Effo
Hrm ... Maybe it's just me, but it seems to work when I don't free(filepathname) and it works fine on smaller directories in my situation ... Thanks for your help!
Kenji
Okay, I just saw your new comment ... Will give that a shot too!
Kenji
ok. another reminder - w/ you new code "filepathname = (char *) malloc(sizeof(char) * strlen(dirname)" the length allocated is not enough.
EffoStaff Effo