Seg Fault - Can not access memory ?

Discussion in 'C Programming' started by Mahendra Kumar Kutare, Oct 17, 2007.

  1. I am trying to implement a webserver with boss-worker model thread
    pool implementation -

    I have a header declaration threadpool.h as -

    typedef struct threadpool_work {
    void (*routine) ();
    void *arg;
    struct threadpool_work *next;
    } threadpool_work_t;

    typedef struct threadpool {
    /* Pool Characteristics */
    int num_threads;
    int max_queue_size;

    int do_not_block_when_full;
    pthread_t *threads;
    int cur_queue_size;
    threadpool_work_t *queue_head;
    threadpool_work_t *queue_tail;

    pthread_mutex_t queue_lock;
    pthread_cond_t queue_not_empty;
    pthread_cond_t queue_not_full;
    pthread_cond_t queue_empty;

    int queue_closed;
    int shutdown;
    } *threadpool_t;

    void threadpool_init(threadpool_t *threadpoolp,
    int num_worker_threads,
    int max_queue_size,
    int do_not_block_when_full);

    int threadpool_add_work(threadpool_t threadpool,
    void *routine,
    void *arg);

    int threadpool_destroy(threadpool_t threadpoolp, int finish);

    void threadpool_thread(threadpool_t threadpool);

    In my server_threadp.c main method , I do following -

    threadpool_t threadpool;

    /* Allocate a pool data structure */
    if ((threadpool = (threadpool_t) malloc(sizeof(struct threadpool))) ==
    NULL) {
    perror("malloc");
    exit(1);
    }

    threadpool_add_work function has following code -

    if (threadpool->cur_queue_size == 0) {
    threadpool->queue_tail = threadpool->queue_head =workp;
    printf("Signal for pthread_cond_wait waiting on
    queue_not_empty:: %d\n", threadpool->queue_not_empty);
    pthread_cond_signal(&threadpool->queue_not_empty);
    } else {
    (threadpool->queue_tail)-> next = workp;
    threadpool->queue_tail = workp;
    }

    For the first HTTP request, it will find threadpool->cur_queue_size ==
    0 and hence will execute the IF block of the above code, but for any
    subsequent request it will execute ELSE block above.

    When I am sending 2nd request, ITS SEGFAULTING at
    (threadpool->queue_tail)-> next = workp;

    I debugged it through gdb and when trying to print following -

    (threadpool->queue_tail)-> next

    it throws - Can not access memory

    Which i believe is because - Memory is not allocated to the structure
    threadpool self element.

    My Question is - When i allocate memory in the begining to threadpool
    structure do i have to still allocate memory for any element which is
    itself in this case. ? If so, how do i do that ?

    Or is there something else which I am missing here ?

    Thanks
    Mahendra
     
    Mahendra Kumar Kutare, Oct 17, 2007
    #1
    1. Advertising

  2. [You really did not need to post your question 3 times. Your posts
    may not appear immediately, that's just the way it is. You may need
    to wait a bit.]

    Mahendra Kumar Kutare wrote:

    > I am trying to implement a webserver with boss-worker model thread
    > pool implementation -


    My first reaction was, "oh no, another off-topic question," but in
    fact yours is perfectly topical.

    <snip>

    > /* Allocate a pool data structure */
    > if ((threadpool = (threadpool_t) malloc(sizeof(struct threadpool))) ==
    > NULL) {


    In C, casting malloc is not required and may hide a problem.
    the preferred way is 'type *p = malloc(*p);'. That way, even if type
    changes, the statement will remain safe. In your case, you could do

    if ((threadpool = malloc(sizeof(*threadpool))) == /*...*/

    > perror("malloc");
    > exit(1);
    > }
    >
    > threadpool_add_work function has following code -
    >
    > if (threadpool->cur_queue_size == 0) {
    > threadpool->queue_tail = threadpool->queue_head =workp;


    What's workp? Is it initialized to a valid threadpool?

    > printf("Signal for pthread_cond_wait waiting on
    > queue_not_empty:: %d\n", threadpool->queue_not_empty);
    > pthread_cond_signal(&threadpool->queue_not_empty);
    > } else {
    > (threadpool->queue_tail)-> next = workp;

    ^^^^^^^
    Because if it isn't - BANG!

    > threadpool->queue_tail = workp;
    > }
    >
    > For the first HTTP request, it will find threadpool->cur_queue_size ==
    > 0 and hence will execute the IF block of the above code, but for any
    > subsequent request it will execute ELSE block above.
    >
    > When I am sending 2nd request, ITS SEGFAULTING at
    > (threadpool->queue_tail)-> next = workp;
    >
    > I debugged it through gdb and when trying to print following -
    >
    > (threadpool->queue_tail)-> next
    >
    > it throws - Can not access memory


    If it does, then the implementors obviously could not spell.
    It should be "cannot" (one word).

    > Which i believe is because - Memory is not allocated to the structure
    > threadpool self element.


    Not so much not allocated as not initialized, I believe.

    > My Question is - When i allocate memory in the begining to threadpool
    > structure do i have to still allocate memory for any element which is
    > itself in this case. ? If so, how do i do that ?


    I am not sure what you mean by "which is itself", but in general,
    allocating alone leaves the value uninitialized. If the value happens
    to be a struct, then its members are unitialized. Accessing an
    uninitialized member may just yield garbage if it happens to be a
    number, but if it is a pointer, the consequences may be more dire,
    as you have experienced.

    > Or is there something else which I am missing here ?


    Probably initializing your workp. Since I do not know where it came
    from, I cannot be sure.

    Peter
     
    Peter Pichler, Oct 18, 2007
    #2
    1. Advertising

  3. Thanks for your response. Sorry for multiple postings.

    Now, coming back to the problem -

    Code for the main method is as below -

    **************************************************************************

    int main (int argc, char *argv[]) {

    threadpool_t threadpool;

    // Code to create INET socket
    // bind, accept and listening socket code

    /* Allocate a pool data structure */
    if ((threadpool = (threadpool_t) malloc(sizeof(struct
    threadpool))) == NULL) {
    perror("malloc");
    exit(1);
    }

    /* Initialize the thread pool */
    threadpool_init(&threadpool, NUM_WORKER_THREADS, MAX_QUEUE_SIZE,
    0);

    while(1) {
    /* Accepting connections from the client */
    clilen = sizeof(cli_addr);
    newsockfd = accept (sockfd, (struct sockaddr *)
    &cli_addr, &clilen);
    if (newsockfd < 0) {
    error ("ERROR on accept");
    }

    threadpool_add_work((void *) threadpool,
    (*accept_requests), (void *) newsockfd);
    }

    return 0;
    }
    **************************************************************************

    void threadpool_init(threadpool_t *threadpoolp,
    int num_worker_threads,
    int max_queue_size,
    int do_not_block_when_full) {

    int i, rtn;
    threadpool_t threadpool;

    /* Allocate a pool data structure */
    if ((threadpool = (threadpool_t) malloc(sizeof(struct
    threadpool))) == NULL) {
    perror("malloc");
    exit(1);
    }

    /* Initialize the fields */
    threadpool->num_threads = num_worker_threads;
    threadpool->max_queue_size = max_queue_size;
    threadpool->do_not_block_when_full = do_not_block_when_full;

    if ((threadpool->threads = (pthread_t *)
    malloc(sizeof(pthread_t) *num_worker_threads)) == NULL) {
    perror("malloc");
    exit(1);
    }

    threadpool->cur_queue_size = 0;
    threadpool->queue_head = NULL;
    threadpool->queue_tail = NULL;

    threadpool->queue_closed = 0;
    threadpool->shutdown = 0;

    if ((rtn = pthread_mutex_init(&(threadpool->queue_lock), NULL))
    != 0) {
    fprintf (stderr, "pthread_mutex_init %s", strerror(rtn));
    exit(1);
    }

    if ((rtn = pthread_cond_init(&(threadpool->queue_not_empty),
    NULL)) != 0) {
    fprintf (stderr, "pthread_cond_init %s", strerror(rtn));
    exit(1);
    }

    if ((rtn = pthread_cond_init(&(threadpool->queue_not_full),
    NULL)) != 0) {
    fprintf (stderr, "pthread_cond_init %s", strerror(rtn));
    exit(1);
    }

    if ((rtn = pthread_cond_init(&(threadpool->queue_empty), NULL))
    != 0) {
    fprintf (stderr, "pthread_cond_init %s", strerror(rtn));
    exit(1);
    }

    /* Create threads */
    for (i = 0; i != num_worker_threads; i++) {
    if ((rtn = pthread_create(&(threadpool->threads),
    NULL, (void *) (*threadpool_thread),
    (void *) threadpool)) != 0) {
    fprintf (stderr, "pthread_create %d", rtn);
    exit(1);
    }
    }

    *threadpoolp = threadpool;

    **************************************************************************

    void threadpool_thread(threadpool_t threadpool) {

    threadpool_work_t *my_workp;

    for(;;) {
    pthread_mutex_lock(&(threadpool->queue_lock));

    printf("Cur queue size:: %d\n",
    threadpool->cur_queue_size);
    printf("Max queue size:: %d\n",
    threadpool->max_queue_size);
    printf("Cur shutdown :: %d\n",
    (!(threadpool->shutdown)));
    while((threadpool->cur_queue_size == 0) &&
    (!threadpool->shutdown)) {
    printf("Waiting for pthread_cond_wait
    queue_not_empty:: %d\n", threadpool->queue_not_empty);

    pthread_cond_wait(&(threadpool->queue_not_empty),
    &(threadpool->queue_lock));
    }

    if (threadpool->shutdown) {
    pthread_mutex_unlock(&(threadpool->queue_lock));
    pthread_exit(NULL);
    }

    my_workp = threadpool->queue_head;
    threadpool->cur_queue_size--;
    if (threadpool->cur_queue_size == 0) {
    threadpool->queue_head = threadpool->queue_tail = NULL;
    } else {
    threadpool->queue_head = my_workp->next;
    }

    if ((!threadpool->do_not_block_when_full) &&
    (threadpool->cur_queue_size =
    (threadpool->max_queue_size -1))) {

    pthread_cond_signal(&(threadpool->queue_not_full));
    }

    if (threadpool->cur_queue_size == 0) {
    pthread_cond_signal(&(threadpool->queue_empty));
    }

    pthread_mutex_unlock(&(threadpool->queue_lock));
    (*(my_workp->routine))(my_workp->arg);
    free(my_workp);
    }
    }

    **************************************************************************
    int threadpool_add_work(threadpool_t threadpool, void *routine, void
    *arg) {

    threadpool_work_t *workp;
    pthread_mutex_lock(&threadpool->queue_lock);

    if((threadpool->cur_queue_size == threadpool->max_queue_size) &&
    threadpool->do_not_block_when_full) {
    pthread_mutex_unlock(&threadpool->queue_lock);
    return -1;
    }

    while((threadpool->cur_queue_size ==
    threadpool->max_queue_size) &&
    ((threadpool->shutdown || threadpool->queue_closed))) {

    printf("Waiting for pthread_cond_wait queue_not_full::
    %d\n", threadpool->queue_not_full);
    pthread_cond_wait(&threadpool->queue_not_full,
    &threadpool->queue_lock);
    }

    if(threadpool->shutdown || threadpool->queue_closed) {
    printf("Threadpool_add_work::Unlocking mutex:: %d\n",
    threadpool->queue_lock);
    pthread_mutex_unlock(&threadpool->queue_lock);
    return -1;
    }

    > /* Allocate work structure */
    > workp = (threadpool_work_t *) malloc(sizeof(threadpool_work_t));
    > workp->routine = routine;
    > workp->arg = arg;
    > workp->next = NULL;
    >
    > if (threadpool->cur_queue_size == 0) {
    > threadpool->queue_tail = threadpool->queue_head =workp;
    > printf("Signal for pthread_cond_wait waiting on queue_not_empty:: %d\n", threadpool->queue_not_empty);
    > pthread_cond_signal(&threadpool->queue_not_empty);
    > } else {
    > (threadpool->queue_tail)-> next = workp;
    > threadpool->queue_tail = workp;
    > }


    threadpool->cur_queue_size++;
    pthread_mutex_unlock(&threadpool->queue_lock);
    return 1;
    }
    **************************************************************************

    The header file threadpool.h looks as below -

    **************************************************************************
    typedef struct threadpool_work {
    void (*routine) ();
    void *arg;
    struct threadpool_work *next;
    } threadpool_work_t;

    typedef struct threadpool {
    /* Pool Characteristics */
    int num_threads;
    int max_queue_size;

    int do_not_block_when_full;
    pthread_t *threads;
    int cur_queue_size;
    threadpool_work_t *queue_head;
    threadpool_work_t *queue_tail;

    pthread_mutex_t queue_lock;
    pthread_cond_t queue_not_empty;
    pthread_cond_t queue_not_full;
    pthread_cond_t queue_empty;

    int queue_closed;
    int shutdown;
    } *threadpool_t;

    void threadpool_init(threadpool_t *threadpoolp,
    int num_worker_threads,
    int max_queue_size,
    int do_not_block_when_full);

    int threadpool_add_work(threadpool_t threadpool,
    void *routine,
    void *arg);

    int threadpool_destroy(threadpool_t threadpoolp, int finish);

    void threadpool_thread(threadpool_t threadpool);
    **************************************************************************

    Now as can be seen "workp" is allocated structure in the above code and
    initialized the 'routine' to routine, 'arg' to arg and 'next' a pointer
    to NULL respectively.

    > I am not sure what you mean by "which is itself", but in general,
    > allocating alone leaves the value uninitialized. If the value happens
    > to be a struct, then its members are unitialized. Accessing an
    > uninitialized member may just yield garbage if it happens to be a
    > number, but if it is a pointer, the consequences may be more dire,
    > as you have experienced.


    Now i see your point that 'next' which itself is a pointer to struct and
    its members are not initialized. But threadpool->queue_tail which has
    struct element 'next' is NOT initialized to which I am trying to
    allocate 'workp'.

    This threadpool is passed in the declared in the beginning of the main
    method and then allocated before calling thread_init(....).

    So how do I initialize

    (threadpool->queue_tail) which points to a structure
    threadpool_work_t and whose element - 'next' is pointer to
    threadpool_work_t ?



    Thanks
    Mahendra


    Peter Pichler wrote:
    > [You really did not need to post your question 3 times. Your posts
    > may not appear immediately, that's just the way it is. You may need
    > to wait a bit.]
    >
    > Mahendra Kumar Kutare wrote:
    >
    >> I am trying to implement a webserver with boss-worker model thread
    >> pool implementation -

    >
    > My first reaction was, "oh no, another off-topic question," but in
    > fact yours is perfectly topical.
    >
    > <snip>
    >
    >> /* Allocate a pool data structure */
    >> if ((threadpool = (threadpool_t) malloc(sizeof(struct threadpool))) ==
    >> NULL) {

    >
    > In C, casting malloc is not required and may hide a problem.
    > the preferred way is 'type *p = malloc(*p);'. That way, even if type
    > changes, the statement will remain safe. In your case, you could do
    >
    > if ((threadpool = malloc(sizeof(*threadpool))) == /*...*/
    >
    >> perror("malloc");
    >> exit(1);
    >> }
    >>
    >> threadpool_add_work function has following code -
    >>
    >> if (threadpool->cur_queue_size == 0) {
    >> threadpool->queue_tail = threadpool->queue_head =workp;

    >
    > What's workp? Is it initialized to a valid threadpool?
    >
    >> printf("Signal for pthread_cond_wait waiting on
    >> queue_not_empty:: %d\n", threadpool->queue_not_empty);
    >> pthread_cond_signal(&threadpool->queue_not_empty);
    >> } else {
    >> (threadpool->queue_tail)-> next = workp;

    > ^^^^^^^
    > Because if it isn't - BANG!
    >
    >> threadpool->queue_tail = workp;
    >> }
    >>
    >> For the first HTTP request, it will find threadpool->cur_queue_size ==
    >> 0 and hence will execute the IF block of the above code, but for any
    >> subsequent request it will execute ELSE block above.
    >>
    >> When I am sending 2nd request, ITS SEGFAULTING at
    >> (threadpool->queue_tail)-> next = workp;
    >>
    >> I debugged it through gdb and when trying to print following -
    >>
    >> (threadpool->queue_tail)-> next
    >>
    >> it throws - Can not access memory

    >
    > If it does, then the implementors obviously could not spell.
    > It should be "cannot" (one word).
    >
    >> Which i believe is because - Memory is not allocated to the structure
    >> threadpool self element.

    >
    > Not so much not allocated as not initialized, I believe.
    >
    >> My Question is - When i allocate memory in the begining to threadpool
    >> structure do i have to still allocate memory for any element which is
    >> itself in this case. ? If so, how do i do that ?

    >
    > I am not sure what you mean by "which is itself", but in general,
    > allocating alone leaves the value uninitialized. If the value happens
    > to be a struct, then its members are unitialized. Accessing an
    > uninitialized member may just yield garbage if it happens to be a
    > number, but if it is a pointer, the consequences may be more dire,
    > as you have experienced.
    >
    >> Or is there something else which I am missing here ?

    >
    > Probably initializing your workp. Since I do not know where it came
    > from, I cannot be sure.
    >
    > Peter
     
    Mahendra Kumar Kutare, Oct 18, 2007
    #3
  4. Mahendra Kumar Kutare

    Don Morris Guest

    Mahendra Kumar Kutare wrote:
    >
    > Thanks for your response. Sorry for multiple postings.
    >
    > Now, coming back to the problem -
    >


    3 issues stand out for me:

    1) You don't check the return value of malloc() when allocating
    workp in threadpool_add_work(). Granted, you don't immediately die
    touching it... but you should check.

    2) As previously noted, make sure you've included stdlib.h and get
    rid of the malloc() casting.

    3) I think your problem is this line in threadpool_thread():
    if ((!threadpool->do_not_block_when_full) &&
    (threadpool->cur_queue_size =
    (threadpool->max_queue_size -1))) {

    If the threadpool is not marked as "do_not_block_when_full", you're
    always setting the size of the circular queue to be the max - 1
    [because I think this is a typo where you have "=" instead of "=="].
    As a result, the queue will erroneously report it has elements when
    you've in fact removed them all... hence you derefence the NULL pointer
    at the tail of the queue and die.

    Since your main() does mark the threadpool via threadpool_init with
    0 for the do_not_block_when_full state... this seems consistent to me.

    Don

    > Code for the main method is as below -
    >
    > **************************************************************************
    >
    > int main (int argc, char *argv[]) {
    >
    > threadpool_t threadpool;
    >
    > // Code to create INET socket
    > // bind, accept and listening socket code
    >
    > /* Allocate a pool data structure */
    > if ((threadpool = (threadpool_t) malloc(sizeof(struct
    > threadpool))) == NULL) {
    > perror("malloc");
    > exit(1);
    > }
    >
    > /* Initialize the thread pool */
    > threadpool_init(&threadpool, NUM_WORKER_THREADS, MAX_QUEUE_SIZE,
    > 0);
    >
    > while(1) {
    > /* Accepting connections from the client */
    > clilen = sizeof(cli_addr);
    > newsockfd = accept (sockfd, (struct sockaddr *)
    > &cli_addr, &clilen);
    > if (newsockfd < 0) {
    > error ("ERROR on accept");
    > }
    >
    > threadpool_add_work((void *) threadpool,
    > (*accept_requests), (void *) newsockfd);
    > }
    >
    > return 0;
    > }
    > **************************************************************************
    >
    > void threadpool_init(threadpool_t *threadpoolp,
    > int num_worker_threads,
    > int max_queue_size,
    > int do_not_block_when_full) {
    >
    > int i, rtn;
    > threadpool_t threadpool;
    >
    > /* Allocate a pool data structure */
    > if ((threadpool = (threadpool_t) malloc(sizeof(struct
    > threadpool))) == NULL) {
    > perror("malloc");
    > exit(1);
    > }
    >
    > /* Initialize the fields */
    > threadpool->num_threads = num_worker_threads;
    > threadpool->max_queue_size = max_queue_size;
    > threadpool->do_not_block_when_full = do_not_block_when_full;
    >
    > if ((threadpool->threads = (pthread_t *)
    > malloc(sizeof(pthread_t) *num_worker_threads)) == NULL) {
    > perror("malloc");
    > exit(1);
    > }
    >
    > threadpool->cur_queue_size = 0;
    > threadpool->queue_head = NULL;
    > threadpool->queue_tail = NULL;
    >
    > threadpool->queue_closed = 0;
    > threadpool->shutdown = 0;
    >
    > if ((rtn = pthread_mutex_init(&(threadpool->queue_lock), NULL))
    > != 0) {
    > fprintf (stderr, "pthread_mutex_init %s", strerror(rtn));
    > exit(1);
    > }
    >
    > if ((rtn = pthread_cond_init(&(threadpool->queue_not_empty),
    > NULL)) != 0) {
    > fprintf (stderr, "pthread_cond_init %s", strerror(rtn));
    > exit(1);
    > }
    >
    > if ((rtn = pthread_cond_init(&(threadpool->queue_not_full),
    > NULL)) != 0) {
    > fprintf (stderr, "pthread_cond_init %s", strerror(rtn));
    > exit(1);
    > }
    >
    > if ((rtn = pthread_cond_init(&(threadpool->queue_empty), NULL))
    > != 0) {
    > fprintf (stderr, "pthread_cond_init %s", strerror(rtn));
    > exit(1);
    > }
    >
    > /* Create threads */
    > for (i = 0; i != num_worker_threads; i++) {
    > if ((rtn = pthread_create(&(threadpool->threads),
    > NULL, (void *) (*threadpool_thread),
    > (void *) threadpool)) != 0) {
    > fprintf (stderr, "pthread_create %d", rtn);
    > exit(1);
    > }
    > }
    >
    > *threadpoolp = threadpool;
    >
    > **************************************************************************
    >
    > void threadpool_thread(threadpool_t threadpool) {
    >
    > threadpool_work_t *my_workp;
    >
    > for(;;) {
    > pthread_mutex_lock(&(threadpool->queue_lock));
    >
    > printf("Cur queue size:: %d\n",
    > threadpool->cur_queue_size);
    > printf("Max queue size:: %d\n",
    > threadpool->max_queue_size);
    > printf("Cur shutdown :: %d\n",
    > (!(threadpool->shutdown)));
    > while((threadpool->cur_queue_size == 0) &&
    > (!threadpool->shutdown)) {
    > printf("Waiting for pthread_cond_wait
    > queue_not_empty:: %d\n", threadpool->queue_not_empty);
    >
    > pthread_cond_wait(&(threadpool->queue_not_empty),
    > &(threadpool->queue_lock));
    > }
    >
    > if (threadpool->shutdown) {
    > pthread_mutex_unlock(&(threadpool->queue_lock));
    > pthread_exit(NULL);
    > }
    >
    > my_workp = threadpool->queue_head;
    > threadpool->cur_queue_size--;
    > if (threadpool->cur_queue_size == 0) {
    > threadpool->queue_head = threadpool->queue_tail = NULL;
    > } else {
    > threadpool->queue_head = my_workp->next;
    > }
    >
    > if ((!threadpool->do_not_block_when_full) &&
    > (threadpool->cur_queue_size =
    > (threadpool->max_queue_size -1))) {
    >
    > pthread_cond_signal(&(threadpool->queue_not_full));
    > }
    >
    > if (threadpool->cur_queue_size == 0) {
    > pthread_cond_signal(&(threadpool->queue_empty));
    > }
    >
    > pthread_mutex_unlock(&(threadpool->queue_lock));
    > (*(my_workp->routine))(my_workp->arg);
    > free(my_workp);
    > }
    > }
    >
    > **************************************************************************
    > int threadpool_add_work(threadpool_t threadpool, void *routine, void
    > *arg) {
    >
    > threadpool_work_t *workp;
    > pthread_mutex_lock(&threadpool->queue_lock);
    >
    > if((threadpool->cur_queue_size == threadpool->max_queue_size) &&
    > threadpool->do_not_block_when_full) {
    > pthread_mutex_unlock(&threadpool->queue_lock);
    > return -1;
    > }
    >
    > while((threadpool->cur_queue_size ==
    > threadpool->max_queue_size) &&
    > ((threadpool->shutdown || threadpool->queue_closed))) {
    >
    > printf("Waiting for pthread_cond_wait queue_not_full::
    > %d\n", threadpool->queue_not_full);
    > pthread_cond_wait(&threadpool->queue_not_full,
    > &threadpool->queue_lock);
    > }
    >
    > if(threadpool->shutdown || threadpool->queue_closed) {
    > printf("Threadpool_add_work::Unlocking mutex:: %d\n",
    > threadpool->queue_lock);
    > pthread_mutex_unlock(&threadpool->queue_lock);
    > return -1;
    > }
    >
    >> /* Allocate work structure */
    >> workp = (threadpool_work_t *) malloc(sizeof(threadpool_work_t));
    >> workp->routine = routine;
    >> workp->arg = arg;
    >> workp->next = NULL;
    >>
    >> if (threadpool->cur_queue_size == 0) {
    >> threadpool->queue_tail = threadpool->queue_head =workp;
    >> printf("Signal for pthread_cond_wait waiting on
    >> queue_not_empty:: %d\n", threadpool->queue_not_empty);
    >> pthread_cond_signal(&threadpool->queue_not_empty);
    >> } else {
    >> (threadpool->queue_tail)-> next = workp;
    >> threadpool->queue_tail = workp;
    >> }

    >
    > threadpool->cur_queue_size++;
    > pthread_mutex_unlock(&threadpool->queue_lock);
    > return 1;
    > }
    > **************************************************************************
    >
    > The header file threadpool.h looks as below -
    >
    > **************************************************************************
    > typedef struct threadpool_work {
    > void (*routine) ();
    > void *arg;
    > struct threadpool_work *next;
    > } threadpool_work_t;
    >
    > typedef struct threadpool {
    > /* Pool Characteristics */
    > int num_threads;
    > int max_queue_size;
    >
    > int do_not_block_when_full;
    > pthread_t *threads;
    > int cur_queue_size;
    > threadpool_work_t *queue_head;
    > threadpool_work_t *queue_tail;
    >
    > pthread_mutex_t queue_lock;
    > pthread_cond_t queue_not_empty;
    > pthread_cond_t queue_not_full;
    > pthread_cond_t queue_empty;
    >
    > int queue_closed;
    > int shutdown;
    > } *threadpool_t;
    >
    > void threadpool_init(threadpool_t *threadpoolp,
    > int num_worker_threads,
    > int max_queue_size,
    > int do_not_block_when_full);
    >
    > int threadpool_add_work(threadpool_t threadpool,
    > void *routine,
    > void *arg);
    >
    > int threadpool_destroy(threadpool_t threadpoolp, int finish);
    >
    > void threadpool_thread(threadpool_t threadpool);
    > **************************************************************************
    >
    > Now as can be seen "workp" is allocated structure in the above code and
    > initialized the 'routine' to routine, 'arg' to arg and 'next' a pointer
    > to NULL respectively.
    >
    > > I am not sure what you mean by "which is itself", but in general,
    > > allocating alone leaves the value uninitialized. If the value happens
    > > to be a struct, then its members are unitialized. Accessing an
    > > uninitialized member may just yield garbage if it happens to be a
    > > number, but if it is a pointer, the consequences may be more dire,
    > > as you have experienced.

    >
    > Now i see your point that 'next' which itself is a pointer to struct and
    > its members are not initialized. But threadpool->queue_tail which has
    > struct element 'next' is NOT initialized to which I am trying to
    > allocate 'workp'.
    >
    > This threadpool is passed in the declared in the beginning of the main
    > method and then allocated before calling thread_init(....).
    >
    > So how do I initialize
    >
    > (threadpool->queue_tail) which points to a structure
    > threadpool_work_t and whose element - 'next' is pointer to
    > threadpool_work_t ?
    >
    >
    >
    > Thanks
    > Mahendra
    >
    >
    > Peter Pichler wrote:
    >> [You really did not need to post your question 3 times. Your posts
    >> may not appear immediately, that's just the way it is. You may need
    >> to wait a bit.]
    >>
    >> Mahendra Kumar Kutare wrote:
    >>
    >>> I am trying to implement a webserver with boss-worker model thread
    >>> pool implementation -

    >>
    >> My first reaction was, "oh no, another off-topic question," but in
    >> fact yours is perfectly topical.
    >>
    >> <snip>
    >>
    >>> /* Allocate a pool data structure */
    >>> if ((threadpool = (threadpool_t) malloc(sizeof(struct threadpool))) ==
    >>> NULL) {

    >>
    >> In C, casting malloc is not required and may hide a problem.
    >> the preferred way is 'type *p = malloc(*p);'. That way, even if type
    >> changes, the statement will remain safe. In your case, you could do
    >>
    >> if ((threadpool = malloc(sizeof(*threadpool))) == /*...*/
    >>
    >>> perror("malloc");
    >>> exit(1);
    >>> }
    >>>
    >>> threadpool_add_work function has following code -
    >>>
    >>> if (threadpool->cur_queue_size == 0) {
    >>> threadpool->queue_tail = threadpool->queue_head =workp;

    >>
    >> What's workp? Is it initialized to a valid threadpool?
    >>
    >>> printf("Signal for pthread_cond_wait waiting on
    >>> queue_not_empty:: %d\n", threadpool->queue_not_empty);
    >>> pthread_cond_signal(&threadpool->queue_not_empty);
    >>> } else {
    >>> (threadpool->queue_tail)-> next = workp;

    >> ^^^^^^^
    >> Because if it isn't - BANG!
    >>
    >>> threadpool->queue_tail = workp;
    >>> }
    >>>
    >>> For the first HTTP request, it will find threadpool->cur_queue_size ==
    >>> 0 and hence will execute the IF block of the above code, but for any
    >>> subsequent request it will execute ELSE block above.
    >>>
    >>> When I am sending 2nd request, ITS SEGFAULTING at
    >>> (threadpool->queue_tail)-> next = workp;
    >>>
    >>> I debugged it through gdb and when trying to print following -
    >>>
    >>> (threadpool->queue_tail)-> next
    >>>
    >>> it throws - Can not access memory

    >>
    >> If it does, then the implementors obviously could not spell.
    >> It should be "cannot" (one word).
    >>
    >>> Which i believe is because - Memory is not allocated to the structure
    >>> threadpool self element.

    >>
    >> Not so much not allocated as not initialized, I believe.
    >>
    >>> My Question is - When i allocate memory in the begining to threadpool
    >>> structure do i have to still allocate memory for any element which is
    >>> itself in this case. ? If so, how do i do that ?

    >>
    >> I am not sure what you mean by "which is itself", but in general,
    >> allocating alone leaves the value uninitialized. If the value happens
    >> to be a struct, then its members are unitialized. Accessing an
    >> uninitialized member may just yield garbage if it happens to be a
    >> number, but if it is a pointer, the consequences may be more dire,
    >> as you have experienced.
    >>
    >>> Or is there something else which I am missing here ?

    >>
    >> Probably initializing your workp. Since I do not know where it came
    >> from, I cannot be sure.
    >>
    >> Peter
     
    Don Morris, Oct 18, 2007
    #4
  5. Hi Don and Peter,

    Thanks for your responses.

    I have modified my code to reflect 1 and 2.

    The biggest issue was as mentioned in point 3. Rather than comparing (==) the
    code was equating (=) that made the code work well.

    I executed the code with the above changes and able to read the static html
    files from webserver client code using command - GET /index.html HTTP/1.1

    I have a further doubt about as following -

    A) With above changes, the code did not SEGFAULT and run well. The code to
    process work is as below -

    I would like to know any improvements which can be done on code below specially
    'accept_requests' and 'process_request' method. This code is currently working fine.

    int main (int argc, char *argv[]) {

    //////////
    Code to create socket, bind and listen for incoming connections
    //////////


    /* Keep waiting for connections */
    while(1) {
    /* Accepting connections from the client */
    clilen = sizeof(cli_addr);
    newsockfd = accept (sockfd, (struct sockaddr *) &cli_addr,
    &clilen);
    if (newsockfd < 0) {
    error ("ERROR on accept");
    }
    /* Add work to the queue for threads to work upon */
    threadpool_add_work((void *) threadp, (*accept_requests), (void
    *) newsockfd);
    }

    //////////
    }

    void *accept_requests(void *params) {

    int socket_fd = (int) params;
    pthread_t tid = pthread_self();

    /* Take the data from socket and pass it for processing */
    process_requests(socket_fd);

    printf("Done processing: %d\n", tid);
    pthread_exit(NULL);
    }

    void process_requests(int socket_fd) {

    int n, ret;
    char buffer[256];
    char line[100], method[100], path[100], protocol[100];
    char *file;
    FILE *fp;
    size_t bytes_read;

    /* Initializing the buffer */
    bzero(buffer, 256);
    /* Reading from socket */
    n = read (socket_fd, buffer, 255);
    if (n < 0) {
    error("ERROR reading from socket");
    }

    printf("Client communication message:: %s\n", buffer);

    ret = sscanf(buffer, "%[^ ] %[^ ] %[^ ]", method, path, protocol);
    if (ret != 3) {
    printf(" Bad Request : Can't parse request ");
    }
    file = &(path[1]);
    fp = fopen(file, "r");
    if (fp == NULL) {
    error (" Could not open the file ");
    } else {
    bzero(buffer, 256);
    bytes_read = fread(buffer, sizeof(buffer), 1, fp);

    }

    write(socket_fd, buffer, 256);
    close(socket_fd);
    }

    B) I have typedefs in header file -

    1) threadpool_t which is a pointer variable - pointer to threadpool structure.

    >> typedef struct threadpool {
    >> /* Pool Characteristics */
    >> int num_threads;
    >> int max_queue_size;
    >>
    >> int do_not_block_when_full;
    >> pthread_t *threads;
    >> int cur_queue_size;
    >> threadpool_work_t *queue_head;
    >> threadpool_work_t *queue_tail;
    >>
    >> pthread_mutex_t queue_lock;
    >> pthread_cond_t queue_not_empty;
    >> pthread_cond_t queue_not_full;
    >> pthread_cond_t queue_empty;
    >>
    >> int queue_closed;
    >> int shutdown;
    >> } *threadpool_t;


    2) I declare in my (current modified code) main method -
    threadpool_t threadp;

    which by header definition above => threadp is of type threadpool_t (typedef),
    which is a pointer to struct threadpool.

    This 'threadp' variable is malloced before threadpool_init is called as -

    if ((threadp = malloc(sizeof(threadp))) == NULL) {
    perror("malloc");
    exit(1);
    }

    Hence to threadpool_init function that has following signature -

    void threadpool_init(threadpool_t *threadp,
    int num_worker_threads,
    int max_queue_size,
    int do_not_block_when_full);

    I pass arg1 as '&threadp' - i.e. I am passing the address of memory location of
    'threadp' - i.e. address of memory location of struct-type 'threadpool'

    But, inside 'threadpool_init' method there is local 'threadP' variable of type
    'threadpool_t' as -
    threadpool_t threadP;

    Here again, I malloced the threadP of structure type 'threadpool_t' as -

    if ((threadP = malloc(sizeof(*threadP))) == NULL) {
    perror("malloc");
    exit(1);
    }

    It can be seen that malloc() assignment for 'threadP' has <b>'*threadP'</b> as
    parameter for sizeof() method. However compare this with malloc assignment of
    'threadp' and it can seen that <b>'threadp'</b> is passed as parameter for
    sizeof() method. So essentially once I am passing pointer variable and in
    another case just the variable of type 'threadpool_t'.

    I tried passing just the variable of type 'threadpool_t' i.e. 'threadP' in
    sizeof() function of malloc code above but that causes program to hang indefinitely.

    I am not sure, as why is this difference of behaviour in mallocing ? Why can not
    the 2nd mallocing work as following -

    if ((threadP = malloc(sizeof(threadP))) == NULL) {
    perror("malloc");
    exit(1);
    }

    It should be noticed that that finally in 'threadpool_init' function 'threadP'
    is assigned to '*threadp' as -

    ====>>>> *threadp = threadP; // In the last line of 'threadpool_init' function

    Appreciate your help on this so far.

    Thanks
    Mahendra

    Don Morris wrote:
    > Mahendra Kumar Kutare wrote:
    >>
    >> Thanks for your response. Sorry for multiple postings.
    >>
    >> Now, coming back to the problem -
    >>

    >
    > 3 issues stand out for me:
    >
    > 1) You don't check the return value of malloc() when allocating
    > workp in threadpool_add_work(). Granted, you don't immediately die
    > touching it... but you should check.
    >
    > 2) As previously noted, make sure you've included stdlib.h and get
    > rid of the malloc() casting.
    >
    > 3) I think your problem is this line in threadpool_thread():
    > if ((!threadpool->do_not_block_when_full) &&
    > (threadpool->cur_queue_size =
    > (threadpool->max_queue_size -1))) {
    >
    > If the threadpool is not marked as "do_not_block_when_full", you're
    > always setting the size of the circular queue to be the max - 1
    > [because I think this is a typo where you have "=" instead of "=="].
    > As a result, the queue will erroneously report it has elements when
    > you've in fact removed them all... hence you derefence the NULL pointer
    > at the tail of the queue and die.
    >
    > Since your main() does mark the threadpool via threadpool_init with
    > 0 for the do_not_block_when_full state... this seems consistent to me.
    >
    > Don
    >
    >> Code for the main method is as below -
    >>
    >> **************************************************************************
    >>
    >>
    >> int main (int argc, char *argv[]) {
    >>
    >> threadpool_t threadpool;
    >> // Code to create INET socket
    >> // bind, accept and listening socket code
    >>
    >> /* Allocate a pool data structure */
    >> if ((threadpool = (threadpool_t) malloc(sizeof(struct
    >> threadpool))) == NULL) {
    >> perror("malloc");
    >> exit(1);
    >> }
    >>
    >> /* Initialize the thread pool */
    >> threadpool_init(&threadpool, NUM_WORKER_THREADS, MAX_QUEUE_SIZE,
    >> 0);
    >>
    >> while(1) {
    >> /* Accepting connections from the client */
    >> clilen = sizeof(cli_addr);
    >> newsockfd = accept (sockfd, (struct sockaddr *)
    >> &cli_addr, &clilen);
    >> if (newsockfd < 0) {
    >> error ("ERROR on accept");
    >> }
    >>
    >> threadpool_add_work((void *) threadpool,
    >> (*accept_requests), (void *) newsockfd);
    >> }
    >>
    >> return 0;
    >> }
    >> **************************************************************************
    >>
    >>
    >> void threadpool_init(threadpool_t *threadpoolp,
    >> int num_worker_threads,
    >> int max_queue_size,
    >> int do_not_block_when_full) {
    >>
    >> int i, rtn;
    >> threadpool_t threadpool;
    >>
    >> /* Allocate a pool data structure */
    >> if ((threadpool = (threadpool_t) malloc(sizeof(struct
    >> threadpool))) == NULL) {
    >> perror("malloc");
    >> exit(1);
    >> }
    >>
    >> /* Initialize the fields */
    >> threadpool->num_threads = num_worker_threads;
    >> threadpool->max_queue_size = max_queue_size;
    >> threadpool->do_not_block_when_full = do_not_block_when_full;
    >>
    >> if ((threadpool->threads = (pthread_t *)
    >> malloc(sizeof(pthread_t) *num_worker_threads)) == NULL) {
    >> perror("malloc");
    >> exit(1);
    >> }
    >>
    >> threadpool->cur_queue_size = 0;
    >> threadpool->queue_head = NULL;
    >> threadpool->queue_tail = NULL;
    >>
    >> threadpool->queue_closed = 0;
    >> threadpool->shutdown = 0;
    >>
    >> if ((rtn = pthread_mutex_init(&(threadpool->queue_lock), NULL))
    >> != 0) {
    >> fprintf (stderr, "pthread_mutex_init %s", strerror(rtn));
    >> exit(1);
    >> }
    >>
    >> if ((rtn = pthread_cond_init(&(threadpool->queue_not_empty),
    >> NULL)) != 0) {
    >> fprintf (stderr, "pthread_cond_init %s", strerror(rtn));
    >> exit(1);
    >> }
    >>
    >> if ((rtn = pthread_cond_init(&(threadpool->queue_not_full),
    >> NULL)) != 0) {
    >> fprintf (stderr, "pthread_cond_init %s", strerror(rtn));
    >> exit(1);
    >> }
    >>
    >> if ((rtn = pthread_cond_init(&(threadpool->queue_empty), NULL))
    >> != 0) {
    >> fprintf (stderr, "pthread_cond_init %s", strerror(rtn));
    >> exit(1);
    >> }
    >>
    >> /* Create threads */
    >> for (i = 0; i != num_worker_threads; i++) {
    >> if ((rtn = pthread_create(&(threadpool->threads),
    >> NULL, (void *) (*threadpool_thread),
    >> (void *) threadpool)) != 0) {
    >> fprintf (stderr, "pthread_create %d", rtn);
    >> exit(1);
    >> }
    >> }
    >>
    >> *threadpoolp = threadpool;
    >>
    >> **************************************************************************
    >>
    >>
    >> void threadpool_thread(threadpool_t threadpool) {
    >>
    >> threadpool_work_t *my_workp;
    >>
    >> for(;;) {
    >> pthread_mutex_lock(&(threadpool->queue_lock));
    >>
    >> printf("Cur queue size:: %d\n",
    >> threadpool->cur_queue_size);
    >> printf("Max queue size:: %d\n",
    >> threadpool->max_queue_size);
    >> printf("Cur shutdown :: %d\n",
    >> (!(threadpool->shutdown)));
    >> while((threadpool->cur_queue_size == 0) &&
    >> (!threadpool->shutdown)) {
    >> printf("Waiting for pthread_cond_wait
    >> queue_not_empty:: %d\n", threadpool->queue_not_empty);
    >>
    >> pthread_cond_wait(&(threadpool->queue_not_empty),
    >> &(threadpool->queue_lock));
    >> }
    >>
    >> if (threadpool->shutdown) {
    >> pthread_mutex_unlock(&(threadpool->queue_lock));
    >> pthread_exit(NULL);
    >> }
    >>
    >> my_workp = threadpool->queue_head;
    >> threadpool->cur_queue_size--;
    >> if (threadpool->cur_queue_size == 0) {
    >> threadpool->queue_head = threadpool->queue_tail = NULL;
    >> } else {
    >> threadpool->queue_head = my_workp->next;
    >> }
    >>
    >> if ((!threadpool->do_not_block_when_full) &&
    >> (threadpool->cur_queue_size =
    >> (threadpool->max_queue_size -1))) {
    >>
    >> pthread_cond_signal(&(threadpool->queue_not_full));
    >> }
    >>
    >> if (threadpool->cur_queue_size == 0) {
    >> pthread_cond_signal(&(threadpool->queue_empty));
    >> }
    >>
    >> pthread_mutex_unlock(&(threadpool->queue_lock));
    >> (*(my_workp->routine))(my_workp->arg);
    >> free(my_workp);
    >> }
    >> }
    >>
    >> **************************************************************************
    >>
    >> int threadpool_add_work(threadpool_t threadpool, void *routine, void
    >> *arg) {
    >>
    >> threadpool_work_t *workp;
    >> pthread_mutex_lock(&threadpool->queue_lock);
    >>
    >> if((threadpool->cur_queue_size == threadpool->max_queue_size) &&
    >> threadpool->do_not_block_when_full) {
    >> pthread_mutex_unlock(&threadpool->queue_lock);
    >> return -1;
    >> }
    >>
    >> while((threadpool->cur_queue_size ==
    >> threadpool->max_queue_size) &&
    >> ((threadpool->shutdown || threadpool->queue_closed))) {
    >>
    >> printf("Waiting for pthread_cond_wait queue_not_full::
    >> %d\n", threadpool->queue_not_full);
    >> pthread_cond_wait(&threadpool->queue_not_full,
    >> &threadpool->queue_lock);
    >> }
    >>
    >> if(threadpool->shutdown || threadpool->queue_closed) {
    >> printf("Threadpool_add_work::Unlocking mutex:: %d\n",
    >> threadpool->queue_lock);
    >> pthread_mutex_unlock(&threadpool->queue_lock);
    >> return -1;
    >> }
    >>
    >>> /* Allocate work structure */
    >>> workp = (threadpool_work_t *) malloc(sizeof(threadpool_work_t));
    >>> workp->routine = routine;
    >>> workp->arg = arg;
    >>> workp->next = NULL;
    >>>
    >>> if (threadpool->cur_queue_size == 0) {
    >>> threadpool->queue_tail = threadpool->queue_head =workp;
    >>> printf("Signal for pthread_cond_wait waiting on
    >>> queue_not_empty:: %d\n", threadpool->queue_not_empty);
    >>> pthread_cond_signal(&threadpool->queue_not_empty);
    >>> } else {
    >>> (threadpool->queue_tail)-> next = workp;
    >>> threadpool->queue_tail = workp;
    >>> }

    >>
    >> threadpool->cur_queue_size++;
    >> pthread_mutex_unlock(&threadpool->queue_lock);
    >> return 1;
    >> }
    >> **************************************************************************
    >>
    >>
    >> The header file threadpool.h looks as below -
    >>
    >> **************************************************************************
    >>
    >> typedef struct threadpool_work {
    >> void (*routine) ();
    >> void *arg;
    >> struct threadpool_work *next;
    >> } threadpool_work_t;
    >>
    >> typedef struct threadpool {
    >> /* Pool Characteristics */
    >> int num_threads;
    >> int max_queue_size;
    >>
    >> int do_not_block_when_full;
    >> pthread_t *threads;
    >> int cur_queue_size;
    >> threadpool_work_t *queue_head;
    >> threadpool_work_t *queue_tail;
    >>
    >> pthread_mutex_t queue_lock;
    >> pthread_cond_t queue_not_empty;
    >> pthread_cond_t queue_not_full;
    >> pthread_cond_t queue_empty;
    >>
    >> int queue_closed;
    >> int shutdown;
    >> } *threadpool_t;
    >>
    >> void threadpool_init(threadpool_t *threadpoolp,
    >> int num_worker_threads,
    >> int max_queue_size,
    >> int do_not_block_when_full);
    >>
    >> int threadpool_add_work(threadpool_t threadpool,
    >> void *routine,
    >> void *arg);
    >>
    >> int threadpool_destroy(threadpool_t threadpoolp, int finish);
    >>
    >> void threadpool_thread(threadpool_t threadpool);
    >> **************************************************************************
    >>
    >>
    >> Now as can be seen "workp" is allocated structure in the above code
    >> and initialized the 'routine' to routine, 'arg' to arg and 'next' a
    >> pointer to NULL respectively.
    >>
    >> > I am not sure what you mean by "which is itself", but in general,
    >> > allocating alone leaves the value uninitialized. If the value happens
    >> > to be a struct, then its members are unitialized. Accessing an
    >> > uninitialized member may just yield garbage if it happens to be a
    >> > number, but if it is a pointer, the consequences may be more dire,
    >> > as you have experienced.

    >>
    >> Now i see your point that 'next' which itself is a pointer to struct
    >> and its members are not initialized. But threadpool->queue_tail which
    >> has struct element 'next' is NOT initialized to which I am trying to
    >> allocate 'workp'.
    >>
    >> This threadpool is passed in the declared in the beginning of the main
    >> method and then allocated before calling thread_init(....).
    >>
    >> So how do I initialize
    >>
    >> (threadpool->queue_tail) which points to a structure
    >> threadpool_work_t and whose element - 'next' is pointer to
    >> threadpool_work_t ?
    >>
    >>
    >>
    >> Thanks
    >> Mahendra
    >>
    >>
    >> Peter Pichler wrote:
    >>> [You really did not need to post your question 3 times. Your posts
    >>> may not appear immediately, that's just the way it is. You may need
    >>> to wait a bit.]
    >>>
    >>> Mahendra Kumar Kutare wrote:
    >>>
    >>>> I am trying to implement a webserver with boss-worker model thread
    >>>> pool implementation -
    >>>
    >>> My first reaction was, "oh no, another off-topic question," but in
    >>> fact yours is perfectly topical.
    >>>
    >>> <snip>
    >>>
    >>>> /* Allocate a pool data structure */
    >>>> if ((threadpool = (threadpool_t) malloc(sizeof(struct threadpool))) ==
    >>>> NULL) {
    >>>
    >>> In C, casting malloc is not required and may hide a problem.
    >>> the preferred way is 'type *p = malloc(*p);'. That way, even if type
    >>> changes, the statement will remain safe. In your case, you could do
    >>>
    >>> if ((threadpool = malloc(sizeof(*threadpool))) == /*...*/
    >>>
    >>>> perror("malloc");
    >>>> exit(1);
    >>>> }
    >>>>
    >>>> threadpool_add_work function has following code -
    >>>>
    >>>> if (threadpool->cur_queue_size == 0) {
    >>>> threadpool->queue_tail = threadpool->queue_head =workp;
    >>>
    >>> What's workp? Is it initialized to a valid threadpool?
    >>>
    >>>> printf("Signal for pthread_cond_wait waiting on
    >>>> queue_not_empty:: %d\n", threadpool->queue_not_empty);
    >>>> pthread_cond_signal(&threadpool->queue_not_empty);
    >>>> } else {
    >>>> (threadpool->queue_tail)-> next = workp;
    >>> ^^^^^^^
    >>> Because if it isn't - BANG!
    >>>
    >>>> threadpool->queue_tail = workp;
    >>>> }
    >>>>
    >>>> For the first HTTP request, it will find threadpool->cur_queue_size ==
    >>>> 0 and hence will execute the IF block of the above code, but for any
    >>>> subsequent request it will execute ELSE block above.
    >>>>
    >>>> When I am sending 2nd request, ITS SEGFAULTING at
    >>>> (threadpool->queue_tail)-> next = workp;
    >>>>
    >>>> I debugged it through gdb and when trying to print following -
    >>>>
    >>>> (threadpool->queue_tail)-> next
    >>>>
    >>>> it throws - Can not access memory
    >>>
    >>> If it does, then the implementors obviously could not spell.
    >>> It should be "cannot" (one word).
    >>>
    >>>> Which i believe is because - Memory is not allocated to the structure
    >>>> threadpool self element.
    >>>
    >>> Not so much not allocated as not initialized, I believe.
    >>>
    >>>> My Question is - When i allocate memory in the begining to threadpool
    >>>> structure do i have to still allocate memory for any element which is
    >>>> itself in this case. ? If so, how do i do that ?
    >>>
    >>> I am not sure what you mean by "which is itself", but in general,
    >>> allocating alone leaves the value uninitialized. If the value happens
    >>> to be a struct, then its members are unitialized. Accessing an
    >>> uninitialized member may just yield garbage if it happens to be a
    >>> number, but if it is a pointer, the consequences may be more dire,
    >>> as you have experienced.
    >>>
    >>>> Or is there something else which I am missing here ?
    >>>
    >>> Probably initializing your workp. Since I do not know where it came
    >>> from, I cannot be sure.
    >>>
    >>> Peter
     
    Mahendra Kumar Kutare, Oct 19, 2007
    #5
  6. Mahendra Kumar Kutare wrote:

    > Hi Don and Peter,
    >
    > Thanks for your responses.


    You are welcome. Please note that top posting is generally frowned upon
    and for a good reason. Most people do not know what top posting is when
    they are doing it, so... top posting is putting your reply above the
    quoted text. The preferred way is snipping everything except the part
    you are replying to and placing your reply below that. That makes it
    much easier to follow what is going on. If you have more than one point
    to reply to, intersperse your replies with the quoted text, like I do now.

    > I have a further doubt about as following -


    Where I live, a "doubt" is an expressed disbelief: "I doubt I can drink
    all night as I used to when I was young." "I have doubts about
    usefulness of gets()."

    Did you mean "question" or "uncertainty"?

    > I would like to know any improvements which can be done on code below
    > specially 'accept_requests' and 'process_request' method. This code is
    > currently working fine.


    You do not specify what kind of improvements are you after, so expect
    the worst.

    The first comment right away: where are your #includes? We (usually) do
    no start with main(), we need to #include some headers first. Especially
    if using non-standard identifiers as you do all over the place.

    > int main (int argc, char *argv[]) {
    >
    > //////////
    > Code to create socket, bind and listen for incoming connections
    > //////////


    If this is in your source, then I doubt :)-)!) it compiles.

    > /* Keep waiting for connections */
    > while(1) {
    > /* Accepting connections from the client */
    > clilen = sizeof(cli_addr);


    I assume cli_addr is declared *somewhere*. It is certainly not in the
    code you provided.

    The name cli_addr suggests it is a variable rather than a type. If that
    is the case, then sizeof does not need parentheses around it.

    > newsockfd = accept (sockfd, (struct sockaddr *) &cli_addr,
    > &clilen);


    More undefined identifiers. So many, in fact, that I had to snip the
    rest of your main().

    > void *accept_requests(void *params) {
    >
    > int socket_fd = (int) params;


    Casting a pointer to int is usually a Bad Idea [tm]. You are not using
    params anywhere else in your accept_requests(), so why not making the
    argument int?

    (7 lines of undefined identifiers snipped.)

    > }


    Your accept_requests() is declared as returning void*, but there is no
    return statement. If you ever use the return value, you are asking for
    trouble. Either return a valid void* or declare the function as void.

    > void process_requests(int socket_fd) {
    >
    > int n, ret;
    > char buffer[256];
    > char line[100], method[100], path[100], protocol[100];


    Mmm, a bit too many buffers, but maybe they'll be needed...

    > char *file;
    > FILE *fp;
    > size_t bytes_read;
    >
    > /* Initializing the buffer */
    > bzero(buffer, 256);


    What's bzero? It is undefined. Perhaps some non-standard function?

    > /* Reading from socket */
    > n = read (socket_fd, buffer, 255);


    (Missing headers notwithstanding...) What if you decide one day to
    decrease buffer to 100? You will need to browse through your code and
    fix all references like this one. On the other hand, using sizeof buffer
    instead of a magic number would take care of that automagically.

    > ret = sscanf(buffer, "%[^ ] %[^ ] %[^ ]", method, path, protocol);


    Oooh, sscanf! You are braver than I am :)

    > file = &(path[1]);
    > fp = fopen(file, "r");


    Why not just path + 1? Or simply fopen(path + 1, "r"), thus eliminating
    one variable?

    > if (fp == NULL) {
    > error (" Could not open the file ");
    > } else {
    > bzero(buffer, 256);
    > bytes_read = fread(buffer, sizeof(buffer), 1, fp);
    >
    > }


    Who will close fp?

    > write(socket_fd, buffer, 256);


    Again, use sizeof buffer instead of 256.

    > close(socket_fd);
    > }
    >
    > B) I have typedefs in header file -
    >
    > 1) threadpool_t which is a pointer variable

    ^^^^^^^^
    type
    <snip>
    > 2) I declare in my (current modified code) main method -
    > threadpool_t threadp;

    <snip>
    > This 'threadp' variable is malloced before threadpool_init is called as -
    >
    > if ((threadp = malloc(sizeof(threadp))) == NULL) {


    ITYM
    if ((threadp = malloc(sizeof(*threadp))) == NULL) {
    ....

    > perror("malloc");
    > exit(1);
    > }
    >
    > Hence to threadpool_init function that has following signature -
    >
    > void threadpool_init(threadpool_t *threadp,
    > int num_worker_threads,
    > int max_queue_size,
    > int do_not_block_when_full);
    >
    > I pass arg1 as '&threadp' - i.e. I am passing the address of memory
    > location of 'threadp' - i.e. address of memory location of struct-type
    > 'threadpool'
    >
    > But, inside 'threadpool_init' method there is local 'threadP' variable
    > of type 'threadpool_t' as -
    > threadpool_t threadP;
    >
    > Here again, I malloced the threadP of structure type 'threadpool_t' as -
    >
    > if ((threadP = malloc(sizeof(*threadP))) == NULL) {
    > perror("malloc");
    > exit(1);
    > }
    >
    > It can be seen that malloc() assignment for 'threadP' has
    > <b>'*threadP'</b>


    Correct

    > as parameter for sizeof() method.


    Incorrect. sizeof is not a method. It is an operator.

    > However compare this
    > with malloc assignment of 'threadp' and it can seen that
    > <b>'threadp'</b> is passed as parameter for sizeof() method. So
    > essentially once I am passing pointer variable and in another case just
    > the variable of type 'threadpool_t'.


    Which is wrong, as I indicated above.

    (Snipped the rest, as it was just repetition.)
     
    Peter Pichler, Oct 19, 2007
    #6
  7. "Peter Pichler" <> schrieb im Newsbeitrag
    news:47193277$...
    > Mahendra Kumar Kutare wrote:
    >> I have a further doubt about as following -

    >
    > Where I live, a "doubt" is an expressed disbelief: "I doubt I can drink
    > all night as I used to when I was young." "I have doubts about usefulness
    > of gets()."
    >
    > Did you mean "question" or "uncertainty"?

    This seems to be a common expression in India and should mean what you
    guessed it might.

    Bye, Jojo
     
    Joachim Schmitz, Oct 20, 2007
    #7
  8. On Fri, 19 Oct 2007 23:40:54 +0100, Peter Pichler
    <> wrote:

    > Mahendra Kumar Kutare wrote:


    > > void *accept_requests(void *params) {
    > >
    > > int socket_fd = (int) params;

    >
    > Casting a pointer to int is usually a Bad Idea [tm]. You are not using
    > params anywhere else in your accept_requests(), so why not making the
    > argument int?


    > Your accept_requests() is declared as returning void*, but there is no
    > return statement. If you ever use the return value, you are asking for
    > trouble. Either return a valid void* or declare the function as void.


    #if offtopic == threading
    The OP's code was obviously designed to run under POSIX threads aka
    pthreads, and that requires that a thread-function takes one void* and
    returns void*. If you want to pass anything else you either store it
    (uniquely) in memory and pass a pointer, or nonportably cast your
    small-enough data to void* and back as here. It should return some
    valid value UNLESS you (always) pthread_exit instead as he does,
    although if no other code/thread uses pthread_join to get that return
    value it doesn't matter what it is.
    #endif

    #if boilerplate
    To the OP, and anyone else in a similar situation: if you have
    questions about or problems with the multithreading issues involved in
    a program like this, they are offtopic in comp.lang.c because Standard
    C does not define or require any kind of threading, pthread or other.
    Go to comp.programming.threads instead.

    Issues of C structure, practice, or style are ontopic here, but you
    should either omit the threading-related and therefore offtopic parts,
    or if you can't omit them (e.g. if the question is about them) clearly
    identify and acknowledge their nonstandardness to avoid flamage.
    And it's a good idea to check the FrequentlyAskedQuestions-list first,
    at the usual places and http://c-faq.com -- re-asking a FAQ is
    considered rude. If you don't understand or aren't satisfied with the
    FAQ-answer, explain that specifically. Good luck.
    #endif

    - formerly david.thompson1 || achar(64) || worldnet.att.net
     
    David Thompson, Oct 29, 2007
    #8
    1. Advertising

Want to reply to this thread or ask your own question?

It takes just 2 minutes to sign up (and it's free!). Just click the sign up button to choose a username and then you can ask your own questions on the forum.
Similar Threads
  1. Corrine
    Replies:
    1
    Views:
    442
    Gianni Mariani
    Nov 27, 2003
  2. Michel Rouzic

    Access to data into an array causes seg-fault

    Michel Rouzic, Jul 3, 2005, in forum: C Programming
    Replies:
    8
    Views:
    407
    John Bode
    Jul 4, 2005
  3. Mahendra Kumar Kutare

    Seg Fault - Can not access memory ?

    Mahendra Kumar Kutare, Oct 17, 2007, in forum: C Programming
    Replies:
    0
    Views:
    342
    Mahendra Kumar Kutare
    Oct 17, 2007
  4. Mahendra Kumar Kutare

    Seg Fault - Can not access memory ?

    Mahendra Kumar Kutare, Oct 17, 2007, in forum: C Programming
    Replies:
    0
    Views:
    321
    Mahendra Kumar Kutare
    Oct 17, 2007
  5. johnty

    seg fault in linux but not cygwin

    johnty, Feb 10, 2010, in forum: C Programming
    Replies:
    36
    Views:
    1,399
    Nick Keighley
    Feb 16, 2010
Loading...

Share This Page