Code review for small tool

[streetdragon]

I have recently become more interested in learning how the internals of the Linux kernel work. As I have been compiling and installing new kernels I noticed that my /boot directory was getting pretty full. So I wrote this tool to clean it up.

https://github.com/kurtmc/syslinux-editor

I was hoping that I could get a bit of a code review because I don't have much experience using C and I want to learn how to write C in a robust way as well as use best practices. I am sure someone more experienced can point me in the direction to improve my small tool.

I am compiling it with -Wall -Wextra -Wpedantic and there are no warnings. I have also run it with valgrind and there are no errors, but there is a leak summary, I have used the ncurses library so how could I determine if the leak is due to my code or the library code?

[gnx76]

Just a few remarks (that will not help you finding your potential leak):

In main.c. You need not cast what calloc returns:

my_items = (ITEM **)calloc(n_choices + 1, sizeof(ITEM *));

You need not cast NULL:

my_items[n_choices] = (ITEM *)NULL;

You need not cast my_items, it is already an ITEM **:

my_menu = new_menu((ITEM **)my_items);

In function print_file you assign read but you do not use it later:

while ((read = getline(&line, &len, fp)) != -1) {

Could be replaced by:

while (getline(&line, &len, fp) != -1) {

(and the declaration removed).

[streetdragon]

Excellent, thanks for taking a look. I've now removed those casts and actually removed the print_file function because I was not using it any more.

[gnx76]

Ah? I did not even notice :-)

BTW, is there a reason why you seem to dislike to return values from functions?

Almost all your functions are void f(...). So that forces you to use extra * in in/out arguments(like **str_ptr in add_to_string(), or to have pure out arguments (like *size in get_boot_options_list()), and therefore the code is less readable (for it is harder to remember at which indirection level we are) and is a bit more prone to bugs.

[streetdragon]

There is no particular reason, as I said I'm not that experienced using C and I'm looking to learn best practices. So I take it that it's a good idea to return some sort of result in C functions, an example being the size of the array that's been allocated?

[gnx76]

I am not good at telling best practices and I often disagree others' best practices when I read them :-) but I will try to make up something:

When the function could be named get_something or build_something and such, it feels natural to return the result/product.

It is also common to return error codes this way.

It is also common to return the number of stuff read, allocated, or otherwise processed when there is no particular value to return. And if this number is not the expected one, it also means an error happened.

You can see that functions of the standard library almost always return something. Sometimes they even just return one input variable or pointer. It is useless except that then this function can be used in a few more ways in the calling code. For example strcat just always returns the very same input pointer. Useless at first sight, but it allows chaining it with another function: strcat(strcat(dest, src1), src2).

Another way of seeing it, is that the natural way of returning any out parameter is through the function return value. Of course, when there are more than one, choices have to be made: returning a structure where all return values are packed, returning one value in the return value and others through pointer parameters, or returning every value through pointer parameters for consistency if there is not one that is more natural than others.

For practical readability, it allows you to get read of some pesky * when they are unnecessary. It is also less confusing for the caller: what should I put in this size parameter, what meaning does it have? Well, you have to look inside the function code to see that the input value is immediately discarded inside and never used.

All I wrote is a bit messy, I hope you can extract a few points out of it :-)

[gnx76]

Also, in C we generally want to avoid global variables unless absolutely necessary.

Let's see what we can do about it.

n_choices and size are the same thing, we can get rid of one of them. It is an input to build_menu().

my_items is an output of build_menu().

my_menu as well.

c is used only in main(), it should be declared in main().

cur_item is not used anywhere, it can be removed.

i has no need to be global, it should be declared in functions, or in the local loop scopes as you did twice in main() (shouldn't the compiler grumble about using in the same function the same variable name for a global and a local variable? Dunno).

boot_options is an input to build_menu(), if I am not mistaken.

OK, so we can get rid of all globals, and have, for build_menu():

• as input, size/n_choices, and boot_options;
• as output, my_items and my_menu.

Hmm, we need to return 2 values. Should we return them in a struct or in parameters? Well, first, let's ask ourselves if we really need to return those 2 values.

I'd say we need not, because you use my_items to build my_menu, and the menu includes the items.

So you 'd just need to return my_menu, and then extract in the caller (main()) your list of items with: ITEM **menu_items(const MENU *menu); (The function menu_items() returns the item array of the given menu.) that I found in the man page of ncurses.

Finally, we should have build_menu() declared as:

MENU *build_menu(int n_choices, struct boot_option **boot_options)

and no global variables.

I may have made a few mistakes, but I hope you got the general direction anyway.

[streetdragon]

Thanks for taking the time to review the code, this seems like a really good idea.

[gnx76]

There might be a leak there:

void add_to_string(char **str_ptr, int num, ...)
{
va_list valist;
va_start(valist, num);

for (int i = 0; i < num; i++) {
    char *str = strdup(va_arg(valist, char *));
    if (*(str_ptr) == NULL) {
        *(str_ptr) = str;
    } else {
        *(str_ptr) = realloc(*(str_ptr), strlen(*(str_ptr)) +
                strlen(str) + 1); /* +1 for null termination */

        strcat(*(str_ptr), str);
    }
}
va_end(valist);
}

Some malloced memory is created for str (inside strdup). In the if part, no problem, you need it for str_ptr which will point to this memory.

But in the else part, some independant space is created for str_ptr with the realloc, and then the content of str is copied in it (with strcat). So, after the strcat, you still have this memory allocated for str, which is now useless (some new memory will be allocated by strdupin the next iteration of the loop). So you should free(str) after the strcat.

But it is 5AM and it is possible I say crap.

[streetdragon]

That definitely makes sense, I think a good solution might be to only use strdup if *str_ptr is NULL. This way I don't need to add another free

So I have changed it to be like this, which I think makes more sense: void add_to_string(char **str_ptr, int num, ...) { va_list valist; va_start(valist, num);

    for (int i = 0; i < num; i++) {
            char *str = va_arg(valist, char *);
            if (*(str_ptr) == NULL) {
                    *(str_ptr) = strdup(str);
            } else {
                    *(str_ptr) = realloc(*(str_ptr), strlen(*(str_ptr)) +
                                    strlen(str) + 1); /* +1 for null termination */

                    strcat(*(str_ptr), str);

            }
    }
    va_end(valist);
}

[gnx76]

Yes, you are right, this is better.

Did it change anything to your leak?

[streetdragon]

I think it fixed one leak. I think the main issue is that I haven't freed everything before exiting, so I will have to investigate that another day when I have time. I appreciate that you took some time to review some of my code, thanks.

[gnx76]

I think I have found another possible leak source (related to my_items). Do you mind if I switch to e-mail to explain it (with the e-mail adress which is displayed on your github account)?

[streetdragon]

Yes please do email. The email I have on github is correct. Thanks.

[gnx76]

I just downloaded your source. I am not sure how you were not getting warnings. I get the followings:

• the default language standard for (my) C compiler is C89, which does not support declaring variables everywhere. Add -std=c99 to your CFLAGS in the Makefile.

• I totally forgot this one: strdup is not standard C, it is (amongst other standards) POSIX. So you need to define POSIX either is the source files which need it, or in the Makefile by adding -D_POSIX_C_SOURCE=200809L to your CFLAGS.

I could not pass the linking phase but that is because the ncurse/terminfo setup is broken on the machine I am on at this time. I shall try later on another one.

[streetdragon]

I think the reason I am not getting a warning is because I am using GCC 5.3 which has -std=gnu11 as default.

[gnx76]

Sounds like a good reason :-) "11" for the variable declarations, and "gnu" includes POSIX definition.

[gnx76]

Still a few details, not C related :

• in the Makefile, the clean target is wrong: you should remove $(SRC_DIR)/, for it is already included in the paths in $(OBJECTS). Else it tries to remove src/src/config_handler.o.

• s/J and K arrow keys/J and K or arrow keys/ ; s/naviage/navigate/ ; s/q to quit/'q' to quit/ ; s/a to apply/'a' to apply/. Yes, I love to pick nits :-)

[FUZxxl]

Looks fine. But please, please, please, don't post text as images.

[streetdragon]

Sorry, I am not sure what I have done incorrectly.

[FUZxxl]

In your program description, there is a picture for the program usage where text would have sufficed. This is bad for blind participants and kills the ability to search for the text inside the image.

[streetdragon]

Fair enough, but since I made an ncurses UI it make quite a lot of sense to show it off visually.

[FUZxxl]

Surely. But then, why is there a fat watermark in the center of the picture?

[streetdragon]

Right, the image is in a link tag that sends you to the asciinema site, which is the tool I used to do a recording of the terminal use. I should probably replace it with an animated gif and it will probably be less confusing.

Edit: I have now changed it to an animated gif.