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.)