Jump to content
  • Advertisement
Sign in to follow this  
wannabeprogrammer

Is this C code using pointers valid?

This topic is 1259 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

If you intended to correct an error in the post then please contact us.

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
Advertisement

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
Sign in to follow this  

  • Advertisement
×

Important Information

By using GameDev.net, you agree to our community Guidelines, Terms of Use, and Privacy Policy.

We are the game development community.

Whether you are an indie, hobbyist, AAA developer, or just trying to learn, GameDev.net is the place for you to learn, share, and connect with the games industry. Learn more About Us or sign up!

Sign me up!