Demonstration of pthread calls

Question

Please review for any unnecessary casting, memory leaks, wrong use of pthread call, or validation problems in the given code.

/**********************************************************************
*       FILENAME :thread_1.c
*       DESCRIPTION:Contains Code for a program that demonstrates the
*       use of pthread calls.The created threads will accept an
*   argument and return the argument after adding a number. 
****************************************************************************/
#include <pthread.h>
#include <stdio.h>
#include <stdlib.h>
void *thread_entry(void *arg)
{

 int *p;
 p=(int*)arg;
 *p = (*p) + 10;
 pthread_exit(p);   
}

int main(int argc, char* argv[])
{
 pthread_t* threads;
 int t,ret,numthreads = 0,thread_success = 0,*iptr;
 void* thread_result;
 if ( argc < 2 )
 {
    printf("Less number of Args -Pl. enter a valid number of threads to be created\n");
    exit(1);
}
numthreads = atoi(argv[1]);
if ( 0 > numthreads )
{
    printf("Pl. enter a valid number of threads to be created\n");
    exit(1);
}
threads = calloc(numthreads,  sizeof(pthread_t));
if (NULL == threads)
{
    printf("Failed to alocate memory in malloc\n");
    exit(1);
}
/*Creating the Threads*/
for(t=0; t < numthreads; t++)
{
    printf("Creating Thread\n");
    iptr = (int*)malloc(sizeof(int));
    *iptr = t;
    ret  = pthread_create(&threads[t],NULL,thread_entry,(void*)iptr);
    if(ret )
    {
        printf("Printf Error Creating Thread");
        free(iptr);
    }
    else
    {
        thread_success++;
    }
}
 //Now waiting for all the threads
   //thread_success is the number of successfully created threads
for(t=0; t < thread_success; t++)
{
    ret  = pthread_join(threads[t],&thread_result);
    if (ret)
    {
        printf("Error joining a thread");
    }
    else
    {
        printf("Joined thread returned %d\n", *((int*)thread_result));
        free(thread_result);
    }
}
  free(threads);
  return 0;
}

Answer 1

Align your code better.
It probably has tabs (replace them with space before pasting) in it that make reading it hard on a website.

Please declare one variable per line:

int t,ret,numthreads = 0,thread_success = 0,*iptr;

You are not saving anything be doing this. But you are making it harder to read fot the next person. Anyway when you get a job (sorry I assume you are in college (forgive me if I am wrong)) no company coding standard will let you get away with it. So best not to get in the habit to start with.

Your detection of bad input is spread over two print statements. Do all the tests that make the input bad in one place then use one print statements. Whose normal content is a description of usage. If you want to be fancy describe what they did wrong. But most people just do it wrong in the first place to see the useage comment.

if ( argc < 2 )
 {
    printf("Less number of Args -Pl. enter a valid number of threads to be created\n");
    exit(1);
}
numthreads = atoi(argv[1]);
if ( 0 > numthreads )
{
    printf("Pl. enter a valid number of threads to be created\n");
    exit(1);
}

// I would have done:

if (( argc < 2 ) || ((numthreads = atoi(argv[1])) == 0))
{
    printf("Usage: %s <number>\nThe number represents the number of threads to start\n", argv[0]);
    exit(1);
}

Your only bug (that I can see) next:

ret  = pthread_create(&threads[t],NULL,thread_entry,(void*)iptr);

You can't see it until you read further and note you are joining on threads[t] even if not all the threads worked. So you need some method to track which threads actually failed and pass that information forward to the join section (what I did below) or you need to re-use the t slot in the thread array (If you loop over the array using thread_success as the limit then you can not leave any blank slots in the array).

typedef struct ThreadAndData
{
    pthread_t   thread;       // Keep all thread info in one place
    int         value;        // That way the master can potentially look at the value
    int         status;       // while the thread is running.
} ThreadAndData;
threads = calloc(numthreads,  sizeof(ThreadAndData));


threads[t].status  = pthread_create(&threads[t].thread,NULL,thread_entry,(void*)&threads[t].value);
if(threads[t].status)
{
    printf("Printf Error Creating Thread");
}


// Now in the joining phase.
// You only need to to join on threads that have a 0 status.
if (thread[t].status == 0)
{
    ret  = pthread_join(threads[t].thread,&thread_result);
}