Generic linked list code C

Question

I have created this code: implementation of a linked list in C.

To test it I created a label struct which contains a string and int variables.

The code is commented well and tested before uploaded to here.

I would like to hear your opinion about it, also I'm not that sure the free and malloc are working well (I used DrMemory and it showed no mistakes but memory is a "leaky" thing (lol)).

#include <stdio.h>
#include <string.h>
#include <stdlib.h>
/*label */
/*The label is managed by lnode, which contains the address-string(8-bits)
 and the label-name. */
 typedef struct label{
 int address; /* The address number. */
 char* name; /* The name of the label. */
 }label;
/*Label implementation*/
/*Create label object*/
 label* createLabel(int address, char* name)
 {
 int len;
 label* newlabel;
 if(NULL == name){
 return NULL;
 }
 len = strlen(name)+1;
 newlabel = (label*)malloc(sizeof(label));
 if(NULL == (newlabel->name = (char*)malloc(sizeof(char)*len))){
 return NULL;
 }
 /*copy name to new-label name*/
 strcpy(newlabel->name, name);
 /*set address in newlabel*/
 newlabel->address = address;
 return(newlabel);
 }
/*delete label object*/
 void deleteLabel(void* ob ){
 label *lbl;
 if(NULL == ob){
 return;
 }
 lbl = (label*)ob;
 /*delete the name*/
 free(lbl->name);
 /*delete the Label object*/
 free(lbl);
 }
/*Print Label-objects --> for Testing*/
/*Allocate a string and fill with label data*/
 void printLabel(void* ob){
 label* obl= (label*)ob;
 printf("%s%d, label: %-10s, ","address: ",obl->address,obl->name);
 }
/*gnode*/
/*generic-node struct to be used in a generic list*/
 struct gnode{
 void *value; /*pointer to generic-node */
 struct gnode* next; /*pointer to next node */
 }typedef gnode;
/*createGnode*/
/*create a gnode to be added to a generic-list*/
 gnode* createGnode( void* value, gnode* next){
 gnode* newnode=NULL;
 if(NULL == value){
 return NULL;
 }
 if (NULL == (newnode = (gnode*)malloc(sizeof(gnode)))) {
 return NULL;
 }
 /*add the value*/
 newnode->value = value;
 /*set next value to be NULL --> All added gnode are
 added at the end of the list and become the last node */
 newnode->next = next;
 return(newnode);
 }
/*freeGnode*/
/*free the allocated gnode, using a function to delete the pointed value */
 void freeGnode(gnode* nodetodelete, void (*deletefunc)( void*)){
 if(NULL == nodetodelete){
 return;
 }
 /*delete the value, pointed by void* value */
 deletefunc(nodetodelete->value);
 /*free the allocated gnode*/
 free(nodetodelete);
 }
/*insertGnode --> insert gnode to list*/
 int insertGnode(gnode** glist, gnode* newnode)
 {
 gnode* curr; /*pointer to current node in list*/
 if(NULL == newnode){
 return -1; /*no item to insert*/
 }
 /*Empty list*/
 else if(NULL == *glist){
 *glist = newnode;
 return 1;
 }
 /*add newnode in last place in list*/
 /*Get to last node*/
 for( curr = *glist; curr->next != NULL; curr = curr->next);
 /*Add the newnode as last node*/
 curr->next = newnode;
 /*return Success*/
 return 1;
 }
/*deleteGenericList*/
/*Parse each generic-node and delete */
 void deleteGenericList(gnode* glist, void (*deleteValue)(void*))
 {
 gnode* curr=NULL;
 gnode* next;
 if(NULL == glist){
 return;
 }
 /*Delete each generic-node*/
 for(curr = glist; curr != NULL; ){
 next = curr->next;
 deleteValue(curr->value); /*free the value objext */
 free(curr); /*free current generic-node */
 curr = next;
 }
 /*Set glist to point at NULL*/
 glist = NULL;
 }/*End deleteGenericList*/
/*writeGlist --> write Gnode to an open-file*/
 void writeGenericList(FILE* fp, gnode* glist, void (*writeValue)(FILE*, void*))
 {
 gnode* curr=NULL; /*curr node in the generic-list*/
 /*check that all arguments are present*/
 if( (NULL == fp)||(NULL==glist)||(NULL==writeValue) ){
 return;
 }
 /*use the writeValue function to write the gnode into the file*/
 for(curr = glist; curr != NULL; curr=curr->next){
 writeValue(fp, curr->value);
 }
 }/*End writeGenericList*/
/*For testing...*/
 void printGlist(gnode* glist, void (*printValue)(void*)){
 gnode *curr;
 if(NULL == glist){
 return;
 }
 for (curr = glist; curr != NULL; curr = curr->next){
 printValue(curr->value);
 }
 }
 void printGlistOLD(gnode* glist, void (*printValue)(void*)){
 gnode *curr;
 if(NULL == glist){
 return;
 }
 printf("%-20s\n", "GenericList: top-To-Bottom");
 for (curr = glist; curr != NULL; curr = curr->next){
 printValue(curr->value);
 }
 }
 int main(int argc, char const *argv[])
 {
 printf("start\n");
 label *label1 = createLabel(100,"hello world");
 label *label2 = createLabel(101,"a");
 label *label3 = createLabel(102,"b");
 label *label4 = createLabel(100,"c");
 label *label5 = createLabel(103,"d");
 label *label6 = createLabel(102,"e");
 gnode* g1 = createGnode(label1,NULL);
 gnode* g2 = createGnode(label2,NULL);
 gnode* g3 = createGnode(label3,NULL);
 gnode* g4 = createGnode(label4,NULL);
 gnode* g6 = createGnode(label6,NULL);
 gnode* g5 = createGnode(label5,g6);
 printf("%d\n",insertGnode(&g1,g2));
 printf("%d\n",insertGnode(&g1,g3));
 printf("%d\n",insertGnode(&g1,g4));
 printf("%d\n",insertGnode(&g1,g5));
 printGlistOLD(g1,printLabel);
 deleteGenericList(g1,deleteLabel);
 return 0;
 }

Thanks!

• CiaPan: There's a memory leak in your code: if allocation of a new label succeeds but allocation for its name fails, you return NULL and do not free() the label structure.

Answer 1

• As I mentioned in the previous review, don't duplicate code in comments. We understand the purpose of strcpy(newlabel->name, name); very well.

• As I mentioned in the previous review, do not cast malloc.

• insertGnode doesn't insert into, but rather appends to a list. Consider renaming.

  You don't seem to worry of the order of the nodes in the list. Consider prepending the new node, as in

  int add_gnode(gnode** glist, gnode* newnode)
  {
   if (newnode == NULL) {
   return -1;
   }
   newnode->next = *glist;
   *glist = newnode;
   return 1;
  }
  

  This approach has two benefits:

  • it doesn't traverse an entire list just to find the tail, and
  • it doesn't special case an empty list.

• printGlist shall call writeGenericList with proper arguments.

• You seem to strive toward OOP approach. Consider making a genericList structure having deletefunc, writeValue, etc as members.

Answer 2

• you consistently cast void * when you should not in C language:

  newlabel = (label*)malloc(sizeof(label));
  

  should be:

  newlabel = malloc(sizeof(label));
  

  and

  void printLabel(void* ob) {
   label* obl = (label*)ob;
  

  should be:

  void printLabel(void* ob) {
   label* obl = ob;
  

• you have a potential memory leak in create_label:

  newlabel = malloc(sizeof(label)); // don't cast malloc
  if (NULL == (newlabel->name = malloc(sizeof(char)*len))) {
   return NULL; // leak previously allocated label
  }
  

• names are inconsistent for gnode management: createGnode inserts node at first position when comment says last, and insertGnode adds it at last position

• freeGnode unconditionnaly frees a node and its content, possibly breaking any link if you remove a node in a list => show at least be explicit in a comment that you cannot remove a node from a list

• in deleteGenericList(gnode* glist, void(*deleteValue)(void*)), the last instruction (glist = NULL;) is a no-op because is only set the local value in the function that will go out of scope at next line
• you should considere to add a function to remove a node from a list