Creating Linked Lists from items in a file

SeniorSE · 09-14-2005, 12:24 PM

Hi all. I am very rusty on my C programming (haven't done it in many many years). I am trying to create a linked list and populate it with data from a file. I am checking to see if a file is present, and if it is, load the data from the file into a linked list of data elements. Here is the "struct" of the linked list:

Code:

typedef struct 
{
	struct ssidlist	*next;
	char * 		ssid;
} ssidlist;

And, here is the code. I am getting many errors, as I can not remember how to manipulate pointers and structs and linked lists. If someone could help me debug the following code, that would be great! I am getting many errors that say that I am using the wrong type of pointers, etc. when passing parameters to the functions. I just have no idea what is wrong. I've tried many iterations, and I'm sure I've messed it up more than helped it.

Here is the relevant part of the program:

Code:

void add(ssidlist **head, char * ssid) {
    ssidlist *tmp;

    if ((tmp = malloc(sizeof(*tmp))) == NULL) {
      (void)exit(1);
    }
    tmp->ssid = ssid;
    tmp->next = head;
    *head = tmp;
}


ssidlist load_ssids(char* file)
{
	ssidlist ssids = NULL;
	int s;
	char * x = NULL;
	FILE *f;
	f = fopen("karma.ssids", "r");
	s = getline(*x, 0, f);
	x[strlen(x)] = '\0';
	while (s != -1)
	{
		add(*ssids, x);
		s = getline(*x, 0, f);
		x[strlen(x)] = '\0';
	}	
	fclose(f);
	
	/* Print out the list once it has been loaded */
	ssidlist *tmp = ssids;
	while (tmp != NULL)
	{
		printf ("ssid list: %s\n", tmp->ssid);
		tmp = tmp->next;
	}
}


int main(int argc, char* argv[])
{
    ssidlist ssids = NULL;

    if (argv[2]) {
	remove("karma.ssids");
    } else {
	/* load currently used ssids */
	ssids = load_ssids("karma.ssids");
    }

    wlancard = argv[1];

    airtap_open(wlancard);
    
    /*
     * Install hooks
     */
    airtap_add_hook(AT_TYPE_MGMT,
                    AT_MGMT_SUBTYPE_PROBE_REQ,
                    AT_DIR_NODS,
                    on_probe_req);

    /*
     * Deliver justice.
     */
    return airtap_loop();
}

Any help you can provide would be greatly appreciated. Thanks.

Mark

Dark_Helmet · 09-14-2005, 03:10 PM

There are a couple of problems that jump out right away. First, you have declarations like this:

Code:

    ssidlist ssids = NULL;

That doesn't match up with what you are using that variable for. NULL is used to set a pointer to an unused value; a flag. ssidlist is defined above as a struct - not a pointer. To have that declaration & assignment make sense, you need to indicate ssids is a pointer to a struct like so:

Code:

    ssidlist *ssids = NULL;

Second, when you call add(), you use:

Code:

add(*ssids, x);

Again, you're not writing what you intend. The asterisk (*) dereferences a pointer. So *<ptr to struct> returns the struct itself. Your declaration of the add() function requires a pointer to a pointer of a struct. So the call above is giving a struct when the function expects something completely different. Your call to add should be this:

Code:

add(&ssids, x);

Lastly, your string handling does a couple of bad things. First:

Code:

x[strlen(x)] = '\0';

I hate to be blunt, but that's a nonsensical or do-nothing statement. strlen() returns the number of characters in a string. How does strlen() know how many characters there are? It counts non-null characters until it reaches a NULL character. Then the code writes a NULL in that location. It doesn't actually accomplish anything. It writes a NULL where there already is one. I'm assuming you're trying to be safe and manually terminating the string, but you'll have to come up with a different method.

Also:

Code:

tmp->ssid = ssid;

This may be OK with the way you use things. getline() will allocate the space for the string automatically with the way you've used it. However, I always prefer to use strcpy() whenever the data gets to its final spot. Say, for instance, you later change your load_ssids() function so that x is a static array of characters (for example: char x[1000]), and getline puts its data into that array. The "tmp->ssid = ssid" code above will break because all of the nodes in the list will be pointing to the memory space x uses. In other words, it will appear as though all the nodes contain the same string. Just a thought.

There may be other issues, but those are the ones that jumped out at me.

spooon · 09-14-2005, 03:18 PM

I changed a few things I saw:

Code:

void add(ssidlist **head, char * ssid) {
    ssidlist *tmp;

    if ((tmp = malloc(sizeof(*tmp))) == NULL) {
      (void)exit(1);
    }
    tmp->ssid = ssid;
    tmp->next = *head;
    *head = tmp;
}


ssidlist *load_ssids(char* file)
{
	ssidlist *ssids = NULL;
	int s;
	char * x = NULL;
	FILE *f;
	f = fopen("karma.ssids", "r");
	s = getline(&x, 0, f);
	/* x[strlen(x)] = '\0'; */
	while (s != -1)
	{
		add(&ssids, x);
		s = getline(&x, 0, f);
		/* x[strlen(x)] = '\0'; */
	}	
	fclose(f);
	
	/* Print out the list once it has been loaded */
	ssidlist *tmp = ssids;
	while (tmp != NULL)
	{
		printf ("ssid list: %s\n", tmp->ssid);
		tmp = tmp->next;
	}

	return ssids;
}


int main(int argc, char* argv[])
{
    ssidlist *ssids = NULL;

    if (argv[2]) {
	remove("karma.ssids");
    } else {
	/* load currently used ssids */
	ssids = load_ssids("karma.ssids");
    }

    wlancard = argv[1];

    airtap_open(wlancard);
    
    /*
     * Install hooks
     */
    airtap_add_hook(AT_TYPE_MGMT,
                    AT_MGMT_SUBTYPE_PROBE_REQ,
                    AT_DIR_NODS,
                    on_probe_req);

    /*
     * Deliver justice.
     */
    return airtap_loop();
}

lowpro2k3 · 09-14-2005, 03:23 PM

edit: spooon beat me to it, use his instead

I think you should process the file, one line at a time using fgets(3) or a similiar line processing function. This is the way I would probably approach the problem. I'm not sure which version of getline(...) you are calling, is it the one from C++ <iostream>?? Or some function you defined? Because it doesn't look like you allocate any space for your char* pointer. This would obviously be a critical error but I would need to know more about your code.

Anyways, the general path I would follow would look like this mixture of C/pseudocode:

Code:

#define MAXLINE 1000

char line[MAXLINE];
FILE* fp;

fp = fopen("yourfile", "r");  /* and error-check */

while(fgets(line, MAXLINE - 1, fp) != EOF)
{
  create new node t
  attach node to list

  t.ssid = (char*) malloc(sizeof(char) * strlen(line) + 1);
  strcpy(t.ssid, line);
}

My implementation is far from perfect (for example, blank nodes will be created for empty lines and such), but it depends on the file structure. But with some modifications that would probably be the approach I take.

SeniorSE · 09-14-2005, 10:25 PM

Thanks for the help. I'll try that as soon as I get a chance. One question, though. What does the '&' do when you put it before a variable?

Mark

Dark_Helmet · 09-14-2005, 10:53 PM

It is the "address of" operator. Think of it as the reverse of the dereference operator.

The & applied to a standard data type (say an int) returns a pointer to that int. Similarly, if you use the & on a pointer, you get a pointer to a pointer. That's what your add() function requires: ssids ** => a pointer to a pointer to a struct.