0

Edit [2016-08-26-001]: I've corrected the errors as pointed out in the comments. As I mentioned below, I quickly grabbed and modified some of the code just to illustrate the problem, sorry :(

The following is a modified extract from my original source (which is still a mess!!!) but it should do:

#define STORAGE_LIMIT_NAME  64
#define DB_INIT_ENTRIES     100

struct entry {
        char fname[STORAGE_LIMIT_NAME];
        char sname[STORAGE_LIMIT_NAME];
};

struct database {
        struct entry *data;
        unsigned int entries;
        unsigned int entrysz;
};

struct database mydb;

mydb.entrysz = sizeof(struct entry);
mydb.entries = DB_INIT_ENTRIES;
mydb.data = malloc(mydb.entrysz * DB_INIT_ENTRIES);

free(mydb.data);

I need to know the following:

1) Am I doing it right? That is, is the memory all being allocated to the heap and thus being freed? I don't want a memory leak.

2) Is there a more elegant solution that doesn't over-complicate things and still allows easy limiting of name lengths? Also, it would be preferably fast.

Sorry if this has been asked before, I've looked all over this site and Google, but all I've found is other people occassionally doing vaguely similar things while asking completely different questions. This was not included in any of the study guides I've looked at online so far. I need to understand this if I want to get better as a programmer and I'm self-taught so I don't have a lecturer to run to. By the way, any links to good free C (not C++) study material is welcomed. Thanks lots.

Improve this question
11
  • 2
    STORAGE_LIMIT_NAME != STORAGE_LIMITS_NAME Commented Aug 26, 2016 at 12:51
  • 3
    for one you are using malloc incorrectly, check doc (stackoverflow.com/questions/7536413/…) Commented Aug 26, 2016 at 12:51
  • 1
    mydb.data = malloc(mydb.entrysz, DB_INIT_ENTRIES); --> mydb.data = malloc(mydb.entrysz * DB_INIT_ENTRIES); Commented Aug 26, 2016 at 12:54
  • Remember to check if malloc() returned NULL. Commented Aug 26, 2016 at 13:06
  • @PaulR: Sorry about that. Was a typo :( Commented Aug 26, 2016 at 15:02

3 Answers 3

Reset to default
2

1) Yes, your code here allocates enough space for an array of DB_INIT_ENTRIES structs, each being sizeof(struct entry) bytes in size:

mydb.entrysz = sizeof(struct entry);
mydb.entries = DB_INIT_ENTRIES;
mydb.data = malloc(mydb.entrysz * DB_INIT_ENTRIES);

This call then frees that entire array:

free(mydb.data);

What you do not show is how to access each struct within the allocated array:

struct entry *  entp;     // Entry pointer
entp = &mydb.data[i];     // Where 0 <= i < mydb.entries


Tangent

As a matter of programming style (which has nothing to do with your question), I prefer to make it explicit that a character array is intended to hold a null-terminated string by adding an explicit +1 to the array size, to wit:

#define STORAGE_LIMIT_NAME  63

struct entry {
    char fname[STORAGE_LIMIT_NAME+1];
    char sname[STORAGE_LIMIT_NAME+1];
};

The explicit +1 makes it clear that the name array ends with an extra NUL character. It also makes STORAGE_LIMIT_NAME define the maximum name length in terms of characters, not its allocation size.


[*] BTW, entry used to be a reserved keyword in early C, but was removed when the language was standardized in 1988.

Improve this answer
Sign up to request clarification or add additional context in comments.

1 Comment

Cool, thanks. That idea with the +1 is something a few people have mentioned to me, but they never mentioned that it could help me limit the actual length too. It's nice to be explained properly! Also, I have no idea about "entry", tbh, but I'll keep in mind to be more careful before I name things (maybe use "my" as a prefix for everything, lol).
1

Yes, there are no leaks. I encourage you to get familiar with Valgrind, which is commonly used for memory leaks.

Also, I would recommend to always typecast your malloc to your specified data type! Don't leave it to the compiler.

And, yes I agree with both the answers above to have +1 bytes to the space for the Null character.

Bonus Material:

Get familiar with GDB, as it is the life saver for many programmers. It's a great debugger and you can be sure that your code runs/does as you expect. I am attaching a cheat sheet for GDB and Valgrind.

GDB

Valgrind

Improve this answer

1 Comment

Thanks for the useful links!!! I didn't cast malloc() because I read somewhere that you only need to cast calloc() and that casting malloc was wrong/bad/pointless. It seems there are differing opinions on this (just to add more confusion to my life). Do you perhaps have a link to some material explaining the situation to me? Thanks
0

1) Yes, there are no leaks.
Your malloc() allocates in the heap mydb.entrysz * DB_INIT_ENTRIES bytes, which can be abstracted as an array of struct entry with size DB_INIT_ENTRIES.

The free(mydb.data) knows how many bytes were dynamically allocated on mydb.data, so it frees all those bytes.

Additionally you should set mydb.data to NULL after the free():
Setting variable to NULL after free

2) If your struct entry stores binary data, I would do exactly as you, but if the names are c strings, I would allocate one more char to the ending '\0' in each array:

struct entry {
    char fname[STORAGE_LIMIT_NAME+1];
    char sname[STORAGE_LIMIT_NAME+1];
};

This way STORAGE_LIMIT_NAME represents the number of bytes you can store.

Improve this answer

1 Comment

Thanks. I would normall set a pointer to NULL after free(), the code was just to illustrate the thing that was confusing me a bit.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.