1

I have a homework to write First Come First Serve and Round Robin simulaion and compare them. I started of creating a linked list for making an event list. I have made insert and print_list functions, but couldn't make it work. My friend told me to use Doubly Linked List so I reacreated the linked list and am trying to make print function work. I had a question about my insertion function as I had mistake in order of statements in while loop. After fixing many errors, the print function prints the same node many times, instead of the whole list. I can't which part of the program is wrong. I think that I have error either in insert function and I point pointers wrong, so I get event pointed to itself or I have the error in print function.

When i deleted this line of the code the print function printed only the current node and nothing else.

newpointer->next = *eventlist;

I think that means that I have problem in this part, but I don't get why.

 if(prevptr == NULL)                  //if putting node at the beginning of the list
 {
     printf("DONEW\n");
     newpointer->next = *eventlist;                    
     *eventlist = newpointer;
 }

Can you please tell me what's wrong with the code?

#include <stdio.h>
#include <stdlib.h>

struct event {
       struct event *next;
       int processid;
       int arrivaltime;
       int cputime;
};
typedef struct event Event;
typedef Event *eventPtr;

Event create_node(int processid, int arrivaltime, int cputime);
void add_event(Event *newpointer, eventPtr *eventlist);
void print_node(Event node);
void print_eventlist(Event *eventlist);
Event get_nextevent(Event *eventlist);

int main(int argc, char* argv[])
{
    int sourcenum,a,b,c,i;
    Event tempevent;
    eventPtr eventlist = NULL;
    print_eventlist(eventlist);
    char* sources,timeSlice;
    if(argc != 3)
    printf("Proper usage is: main.c sources time-slice\n");
    else
    {
        sourcenum = atoi(argv[1]);
        timeSlice = atoi(argv[2]);
        for(i = 0; i < sourcenum ; i++)
        {
              //print_eventlist(&(*eventlist));
              printf("please enter the process-id, arrival-time and cpu-time.\n");
              scanf("%d %d %d",&a, &b, &c);
              printf("something\n");
              tempevent = create_node(a,b,c);
              printf("something\n");
              add_event(&tempevent, &eventlist);
              print_eventlist(eventlist);
        }
        print_eventlist(eventlist);    
    }
    return 0;
}


void add_event(Event *newpointer, eventPtr *eventlist)     //insert
{
     printf("DONE1\n");
     eventPtr currentptr, prevptr;
     printf("DONE2\n");
     prevptr = NULL;
     printf("DONE3\n");
     currentptr = *eventlist;
     printf("DONE4\n");
     while((currentptr != NULL && currentptr->arrivaltime < newpointer->arrivaltime))                  /*on every loop previous and current pointer gets upgraded*/
     {
             printf("DONEW\n");
             prevptr = currentptr;
             currentptr = currentptr->next;   
     }
     printf("DONEW\n");
     if(prevptr == NULL)                  //if putting node at the beginning of the list
     {
         printf("DONEW\n");
         newpointer->next = *eventlist;                    
         *eventlist = newpointer;
     }
     else //putting list between current and previous
     {
         printf("DONEW\n");
         prevptr->next = newpointer;
         newpointer->next = currentptr;
     }
     printf("DONE\n");
}

Event create_node(int processid, int arrivaltime, int cputime)
{
      Event *ptr;
      ptr = malloc(sizeof(Event));
      if(ptr != NULL)
      {
         ptr->processid = processid;       
         ptr->arrivaltime = arrivaltime;    
         ptr->cputime = cputime;
         ptr->next = NULL;
      }
      return *ptr;
}

void print_node(Event node)
{
  printf("process-id = %d\n",node.processid);
  printf("Arrival Time = %d\n",node.arrivaltime);
  printf("CPU time = %d\n",node.cputime);   
}

void print_eventlist(eventPtr currentPtr)                                           /*function receives an eventlist and prints all of the events*/
{
    if(currentPtr == NULL)
    {
        puts("List is empty\n");
    }
    else
    {
        puts("The List is:");
        while(currentPtr != NULL)
        {
            printf("==========================================\n");//list #%d\n",i);
            print_node(*currentPtr);
            printf("\n==========================================\n");
            currentPtr = currentPtr->next;
        }
        puts("NULL\n");
    }
}
    /*int i=0;                                     //This is what I had before
    eventPtr currentptr;
    *currentptr = *eventlist;
    printf("something!!!");
    while(currentptr != NULL)
    {
         i++;
         printf("==========================================\nlist #%d\n",i);
         print_node(*currentptr);
         printf("\n==========================================\n");
         currentptr = currentptr->next;            
    }
}*/

/*Event get_nextevent(Event *eventlist)
{    
    Event newstruct = create_node(eventlist->processid, eventlist->arrivaltime, eventlist->cputime);
    eventlist = eventlist->next;
    return newstruct;  
}*/
GreedyAi
  • 2,709
  • 4
  • 29
  • 55
  • 1
    instead of returning a local stack object in `create_node`, should return a dynamically (heap-allocated) memory which can be manipulated predictably from the rest of the program. ie return `ptr` not `*ptr`.. – amdixon Feb 13 '14 at 07:50
  • I changed it and it worked, but I don't get why? can you please advise me a book or a page to read about memory allocation. I see that I don't fully understand it. – GreedyAi Feb 13 '14 at 08:07
  • 2
    Side Bar: You'll just have to trust me when I say that `eventPtr` adds *nothing* to this code. You should be using `Event *`. It will make it *far* easier to read by anyone versed in C. – WhozCraig Feb 13 '14 at 08:12
  • @WhozCraig I just saw that style today in my book. I will change it as I didn't like it too. Thx for telling. – GreedyAi Feb 13 '14 at 08:16
  • 1
    Check out [local memory address is valid after function return?](http://stackoverflow.com/questions/7115157/local-memory-address-is-valid-after-function-return) and [Returning local data from functions in C and C++ via pointer](http://stackoverflow.com/questions/3127507/returning-local-data-from-functions-in-c-and-c-via-pointer), for a bit more info on local memory scope.. – amdixon Feb 13 '14 at 08:18
  • @vatomargvelashvili no worries. C programmers like seeing asterisks. They're big flags that scream "pointer here" etc. Hiding them in a typedef really only has use for exposing opaque "handles" in an api library, or as a type for functions (i.e. `typedef void (*FnCallback)(int, int,int);`. Proponents say they help with f'ed up multi-var decls because `Type* a,b;` doesn't do what it initially appears to , while `TypePtr a,b;` does. Thats an empty argument, however, as doing it wrong (the former) is all-but-guaranteed to break at compile time anyway, and you won't make that mistake again. – WhozCraig Feb 13 '14 at 08:38
  • +1 for doing your homework (unlike many here...). – michaelmeyer Feb 13 '14 at 12:04

2 Answers2

2

There a a multitude of things to address in this code.

First, you're leaking memory in create_node. That function should be designed to dynamically allocate an event, prepare it with the passed parameters, and return it to the caller as an address to be assigned to a pointer on the caller-side:

Event* create_node(int processid, int arrivaltime, int cputime)
{
    Event *ptr = malloc(sizeof(*ptr));
    if (ptr == NULL)
    {
        perror("Failed to allocate Event.");
        exit(EXIT_FAILURE);
    }

    ptr->processid = processid;
    ptr->arrivaltime = arrivaltime;
    ptr->cputime = cputime;
    ptr->next = NULL;
    return ptr;
}

Next, your add_event function should take an event pointer as the list head, and ideally the address of said-pointer to allow it to be modified on initial-insert. Two schools of thought prevail for insertion functions, (1) always return the list head, and (2) use a pointer-to-pointer for the list head parameter, and modify it via dereference as needed. I prefer the latter, as it makes the code very simple. Even more so in your case since new event creation is done somewhere else.

// link an event on to the end of our linked list.
void add_event(Event** pp, Event* event)
{
    while (*pp)
        pp = &(*pp)->next;
    *pp = event;
}

Once those are taken care of, the source in main() also becomes significantly simpler, taking into account I've also fixed the improper timeSlice variable, which was char and should have been int. More importantly, the logic you had would have a blank list head node, which buys you nothing. You can use a pointer, then call our add_event function and pass its address. It will properly insert the node.

The final product looks like this (with minor mods):

#include <stdio.h>
#include <stdlib.h>

typedef struct event
{
    struct event *next;
    int processid;
    int arrivaltime;
    int cputime;
} Event;

Event* create_node(int processid, int arrivaltime, int cputime);
void add_event(Event** head, Event *event);
void print_node(const Event* node);
void print_eventlist(const Event *eventlist);

int main(int argc, char* argv[])
{
    int sourcenum,a,b,c,i,timeslice;
    Event* events = NULL;

    if(argc != 3)
    {
        printf("Proper usage is: main.c sources time-slice\n");
        exit(EXIT_FAILURE);
    }

    // TODO: these really should be using strtol() for better
    //  error detection. left as exercise for author.
    sourcenum = atoi(argv[1]);
    timeslice = atoi(argv[2]);

    for(i = 0; i < sourcenum ; i++)
    {
        printf("please enter the process-id, arrival-time and cpu-time.\n");
        if (scanf("%d %d %d",&a, &b, &c) == 3)
        {
            add_event(&events, create_node(a,b,c));
            print_eventlist(events);
        }
    }
    return 0;
}

// link an event on to the end of a linked list.
void add_event(Event** pp, Event* event)
{
    while (*pp)
        pp = &(*pp)->next;
    *pp = event;
}

Event* create_node(int processid, int arrivaltime, int cputime)
{
    Event *ptr = malloc(sizeof(*ptr));
    if (ptr == NULL)
    {
        perror("Failed to allocate Event.");
        exit(EXIT_FAILURE);
    }

    ptr->processid = processid;
    ptr->arrivaltime = arrivaltime;
    ptr->cputime = cputime;
    ptr->next = NULL;
    return ptr;
}

void print_node(const Event* node)
{
    if (node != NULL)
    {
        printf("process-id = %d\n",node->processid);
        printf("Arrival Time = %d\n",node->arrivaltime);
        printf("CPU time = %d\n",node->cputime);
    }
    else
    {
        printf("NULL\n");
    }
}

void print_eventlist(const Event* currentPtr)
{
    puts("==========================================");
    while(currentPtr != NULL)
    {
        print_node(currentPtr);
        puts("==========================================");
        currentPtr = currentPtr->next;
    }
    puts("NULL");
    puts("==========================================");//list #%d\n",i);
}

Sample Output

Obtained from a command line requesting 5 entries.

please enter the process-id, arrival-time and cpu-time.
1 1 1 
==========================================
process-id = 1
Arrival Time = 1
CPU time = 1
==========================================
NULL
==========================================
please enter the process-id, arrival-time and cpu-time.
2 2 2 
==========================================
process-id = 1
Arrival Time = 1
CPU time = 1
==========================================
process-id = 2
Arrival Time = 2
CPU time = 2
==========================================
NULL
==========================================
please enter the process-id, arrival-time and cpu-time.
3 3 3 
==========================================
process-id = 1
Arrival Time = 1
CPU time = 1
==========================================
process-id = 2
Arrival Time = 2
CPU time = 2
==========================================
process-id = 3
Arrival Time = 3
CPU time = 3
==========================================
NULL
==========================================
please enter the process-id, arrival-time and cpu-time.
4 4 4
==========================================
process-id = 1
Arrival Time = 1
CPU time = 1
==========================================
process-id = 2
Arrival Time = 2
CPU time = 2
==========================================
process-id = 3
Arrival Time = 3
CPU time = 3
==========================================
process-id = 4
Arrival Time = 4
CPU time = 4
==========================================
NULL
==========================================
please enter the process-id, arrival-time and cpu-time.
5 5 5
==========================================
process-id = 1
Arrival Time = 1
CPU time = 1
==========================================
process-id = 2
Arrival Time = 2
CPU time = 2
==========================================
process-id = 3
Arrival Time = 3
CPU time = 3
==========================================
process-id = 4
Arrival Time = 4
CPU time = 4
==========================================
process-id = 5
Arrival Time = 5
CPU time = 5
==========================================
NULL
==========================================

Anyway, hope it gives you some good ideas.

WhozCraig
  • 65,258
  • 11
  • 75
  • 141
1

Change the type of:

 Event tempevent;

to

 eventPtr tempevent;

The problem you have is that you pass &tempevent and with Event tempevent, this address is always the same. Change the create_node to return an eventPtr too.

eventPtr create_node(int processid, int arrivaltime, int cputime)
{
      Event *ptr;
      ptr = malloc(sizeof(Event));
      if(ptr != NULL)
      {
         ptr->processid = processid;       
         ptr->arrivaltime = arrivaltime;    
         ptr->cputime = cputime;
         ptr->next = NULL;
      }
      return ptr;
}
nmenezes
  • 910
  • 6
  • 12