Sign in to follow this  
wannabeprogrammer

Is this C code using pointers valid?

Recommended Posts

Hi all.

 

I'm trying to write a C function that returns a dynamic list of servers. My pointer memory is a bit rusty so I just wanted to check if the code below is correct. Although the output is as expected, i.e. 3 servers with IP & name, and there appears to be no memory leaks (used Visual Leak Detector) I just want to triple check. Note, I'm just interested in knowing if the pointer usage is correct rather than actual logic or error checking. I'm using VS 2013 express. Thanks in advance.

#include "stdafx.h"
#include <stdio.h>
#include <string.h>
#include <stdlib.h> 

struct serverInfo
{
	char serverName[50];
	char serverIP[20];
};

int getServerListTest(serverInfo** pServers);

int _tmain(int argc, _TCHAR* argv[])
{
	serverInfo *serverList = (serverInfo*)calloc(1, sizeof(serverInfo));
	serverInfo **serverListPtr = &serverList;
	int serverCount = getServerListTest(serverListPtr);
	
	for (int i = 0; i < serverCount; i++)
	{
		printf("ip:%s name:%s\n", (*serverListPtr+i)->serverIP, (*serverListPtr+i)->serverName);
	}
	
	free(serverList);
	serverListPtr = NULL;

	getchar();
	return 0;
}

int getServerListTest(serverInfo** pServers)
{
	serverInfo* current = NULL;
	int serverCount= 0;

	char serverNo[5];
	for (int i = 0; i < 3; i++)
	{
		if (serverCount > 0)
		{
			current = (serverInfo*)realloc((*pServers), sizeof(serverInfo) * (serverCount+1));
			if (current)
			{
				pServers = &current;
			}
			else
			{
				return -1;
			}
		}
		_itoa(serverCount+1, serverNo, 10);
		strcpy((*pServers + serverCount)->serverIP, "10.10.100.10");
		strcat((*pServers + serverCount)->serverIP, serverNo);
		strcpy((*pServers + serverCount)->serverName, "Server");
		strcat((*pServers + serverCount)->serverName, serverNo);
		serverCount++;
	}
	
	return serverCount;
}

Share this post


Link to post
Share on other sites

I voted your answer down FRex because without any concrete examples it is unnecessarily negative.

Saying that it's "obfuscated on purpose" and that "it's not even proper C" is net helpful. Unless you demonstrate how to do it 'properly'.

Just my thoughts.

I am not really an expert in C so I can't offer any advice on how sound/unsound the code is.

However, the code doesn't look awful at all. ;)

Edited by jacmoe

Share this post


Link to post
Share on other sites

 

			current = (serverInfo*)realloc((*pServers), sizeof(serverInfo) * (serverCount+1));
			if (current)
			{
				pServers = &current;
			}
			else
			{
				return -1;
			}
This part is broken.... I think (the logic is confusing).
You reallocate the input data, so it seems that you want to pass that new allocation back out to the caller. If so, it should be:
(*pServers) = (serverInfo*)realloc((*pServers), sizeof(serverInfo) * (serverCount+1));
Anywhere where you're taking the address of a local variable should be cause for great inspection of the logic smile.png

 

 

Careful with this though, if realloc fails and returns a nil pointer you've lost your original pointer and you're screwed! biggrin.png Unless the caller knows that it has to make a copy of the input pointer, which can get complicated fast with more complex situations.

Share this post


Link to post
Share on other sites

i'd suggest the following algo:

 

1. do a pass and get a count of servers

2. malloc the ram for the list

3. do a second pass to populate the list

4. return a pointer to the list

5. perhaps use a separate result parameter for sending back error results (empty list, out of ram, other errors)

6. main() frees the pointer when done.

7. document the list generating subroutine so its CLEAR it allocates memory that the coder must track and free manually.

8. use array notation where possible for clarity.

 

when doing dynamic memory allocations, you want to keep them as simple and infrequent as possible, with either minimum or maximum scope.

 

minimum scope for temporary stuff. never keep ram allocated or a file open any longer than necessary - it cuts down on potential problems.

 

maximum scope for "global" stuff so you are almost always assured that at any point in the code the pointer is valid, and its easy to remember where its not (like program startup and shutdown).  this can cut down on or eliminate the need for runtime checks for valid pointers.

 

things like this can simplify life with pointers.

Share this post


Link to post
Share on other sites


7. document the list generating subroutine so its CLEAR it allocates memory that the coder must track and free manually.

 

you might even want to call your list generating routine something like:

 

create_server_list(*list,*result)

 

and write an explicit matching free routine:

 

free_server_list(*list)

 

which would just call free(*list).

 

that way your API has explicit create() and free() routines, making it more clear that your are creating and destroying a data structure on the heap.

Share this post


Link to post
Share on other sites

Damn, 1 little asterisk breaks it. Thanks for pointing that out. Not sure how it worked as expected anyway!

I did try using array notation but couldn't get it to work, so guess I need to read up about that a bit more unless someone is kind enough to show a quick example.

 

I tagged it as C as I didn't want people suggesting the use of new/delete or vector class etc. I'm using Visual C++ compiler so I confess it may not be *pure C* but for the purposes of this post it served its purpose.

 

FRex, I'm not sure what you mean by the following points

 

This code may read freed memory. - I don't access the pointer again after the call to free

This code may double free. - Not sure how\where I'm doing this?

 

Thanks for the replies and suggestions :)

Share this post


Link to post
Share on other sites


I'm just interested in knowing if the pointer usage is correct rather than actual logic or error checking.

 

i suspect there are logic flaws, hidden by the use of pointer vs array notation, which makes it hard to even spot the probable errors in the pointer notation code.

 

try rewriting it with array notation, and make it actually do something (even dummy calls to get server info). if it still doesn't work, then post that code.

Share this post


Link to post
Share on other sites

I did try using array notation but couldn't get it to work, so guess I need to read up about that a bit more unless someone is kind enough to show a quick example.

 

NOTE: this is off the top of my head... be sure to double check the syntax!

 

basically:

 

 
void main()
{
// -------------- constants -----------------
#define LISTSIZE 3   // in your code your list is hard coded to size 3.
 
//----------- variables ----------------------
serverInfo *list // pointer to your list array
int i;   // loop counter

// ----------- code ---------------------------
list=(serverInfo *)malloc(sizeof(serverInfo)*LISTSIZE);   // allocate your list
// ---------------------  populate list     
for (i=0; i<LISTSIZE; i++)     
    {    
    strcpy(list[i].serverIP, "10.10.100.10");     
    // etc...    
    // IE: list[i].whatever=some_value;    
    }
// ------------------------- print list     
for (i=0; i<LISTSIZE; i++)     
   {    
   printf("%s\n",list[i].serverIP);     
   // and so on....    
   }
// ----------------------- deallocate list
free(list);    // free the RAM
}  
 
 
Edited by Norman Barrows

Share this post


Link to post
Share on other sites

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now

Sign in to follow this