CPU core temperature Unity indicator

Project now on Github: https://github.com/IanCaio/TempI

I just finished a working version of a small Unity indicator in C, that is supposed to take the output of the application "sensors" (sudo apt-get install sensors required) and uses it to monitor the temperature of the CPU.

It creates one indicator that has a small menu, with the name and authorship and a "Quit" button. Then it creates one indicator for each CPU core and displays the temperature of each, also changing the color of the icon depending on the temperature range.

I had to make some harder than average string manipulations and have to say I'm still not comfortable with it. Maybe I just have "buffer overflowphobia". Or maybe I just need to read string.h documentation until I know every function without needing to type man every time.

I tried to structure the program to make it really organized. Tried to divide it in many functions so it would be easier to review single tasks and if they are being done properly.

Both code blocks are a little large for a full review, but if you spot something that is bad practice on C I'd love to hear some feedback. Even if it's related only to a piece of the code. I'll review it myself since it just got of the oven and might have some hidden bugs (couldn't trace memory leaks on valgrind after some point because gtk and valgrind don't get along..).

Header:

```
#define TEMPI_MAX_CORES 4
#define TEMPI_MAX_CHARS 4096
#define TEMPI_COOL 50

//Function Prototypes
void TempI_Free_Everything(); //Frees memory
int TempI_Resolve_Executable_Path(); //Resolves executable path
int TempI_Read_Config(); //Reads config file and sets the variables on TempI_Main
void TempI_Set_Main_Indicator(); //Sets the main gtk app indicator
void TempI_Set_Core_Indicator(); //Sets the cores gtk app indicators
void TempI_Callback_Quit(); //Callback to quit program
int TempI_Number_Of_Cores(); //Returns the number of cores
gint TempI_Update(); //Updates the indicator
int TempI_Get_Cor

This definitely looks like a real project that someone can use or expand. Have you put it on
GitHub?

The symbolic constants defined in TempI.h are following the C Standard.

Some general considerations, while the most common numbers of cores in a CPU chip are
currently 1, 2 or 4, there can also be larger powers of 2 such as 8, 16 or 32. Rather
than using a fixed size array, it might be better to get the number of cores at
run time and then create an array of the appropriate size. The number of icons for
cores could remain the same as long as a modulo is utilized when indexing the
array of core icons.

The size of the program indicates that modularity might be a good thing, consider
breaking the single C file into multiple files and using a makefile to build the
program. If the program gets posted on GitHub, a makefile will be necessary.

There is no test in the program to see if the number of cores exceeds TEMPI_MAX_CORES,
this can lead to unknown results/errors in the program.

Hiding Goto

The macros in debugger.h hide the use of goto. While the use of goto in modern
languages is frowned upon there are legitimate uses for it, such as error handling
such as this code is using it. Hiding goto's is really a bad practice, because
a goto changes the flow of the logic and the flow of the code. When a goto is
hidden in a macro it is VERY difficult to maintain and debug code, in fact almost
impossible unless someone finds the label. The fact that the goto is hidden means
that a programmer may overlook labels if they are scanning the code quickly.

You probably should look into setjmp() and longjmp() for error conditions and error handling. While longjmp() is a form of goto, it is a goto that jumps to a location set previously by setjmp() and is sometimes used to return program
flow to main() to handle any error conditions in a consistent manner (deallocation of resources such as freeing allocated memory and closing files prior to exiting).
It is discussed by this question on stackoverflow.com.

Global Variables

The code contains :

TempI_Main_t TempI_Main;

int main(int argc, char **argv){
...
}

indicating that the variable TempI_Main is a global variable.

Many of us are taught either in school or in our first year at work not to use
global variables. When one uses global variables all changes to the variables that are global are side effects. The down side of side effects are that they can happen anywhere, and are very hard to trace. This makes the code harder to write, maintain, and especially debug.

You can read more about why global variables are considered bad at this question on stackoverflow.com, and a decent discussion of when to use global variables on programmers.com.
If you search for global variables on Google, you will find more diatribes against them.

In the case of this code, TempI_Main should probably be declared in main() and then passed as a parameter to all the functions that use it. This would be more maintainable and easier to debug.

Inconsistent Indentation

In main() The initialization of the TempI_MAIN structure is indented more than
the rest of the code. This is unusual to say the least. Try to keep all of the
indentation at the proper level. There are alternates of initializing structs as well.

One such alternate method is:

TempI_Main TempI_Main = {
        .Delay=2;
        .Log=0;
        .LogFileName=NULL;
        .LogFile=NULL;
        .ExecutablePath=NULL;
        .Cores_Counter=0;
        .Gtk_Indicator_Icon_Path=NULL;
    }

this may give you the indentation you were seeking.

Inconsisten use of Symbolic Constants and Magic Numbers

While there are symbolic constants defined in TempI.h there are places where they aren't used that they could be. In TempI.h there is the following code:

#define TEMPI_MAX_CORES 4
#define TEMPI_MAX_CHARS 4096
#define TEMPI_COOL 50

and then

//CPU Cores
    TempI_CPU_Core Core[TEMPI_MAX_CORES];
    int Cores_Counter; //How many cores are being monitored?
    char *Gtk_Core_Icon_Path[4]; //Path to different icons for the CPU indicator

It's quite possible that Gtk_Core_Icon_Path[0] should be Gtk_Core_Icon_Path[TEMPI_MAX_CORES] and
that the following code in main()

for(int i=0;i<4;i++){
            TempI_Main.Gtk_Core_Icon_Path[i]=NULL;
        }

should be

for(int i = 0; i < TEMPI_MAX_CORES; i++){
            TempI_Main.Gtk_Core_Icon_Path[i] = NULL;
        }

Please note the use of spaces in the for loop, it will help make the code
easier to read and maintain.

It's not clear in the code where the array of TempI_CPU_Core_Struct is initialized. Perhaps the previous loop should be

for(int i = 0; i < TEMPI_MAX_CORES; i++){
            TempI_Main.Gtk_Core_Icon_Path[i] = NULL;
            memset(&TempI_Main.Core[i], 0, sizeof(TempI_CPU_Core_Struct));
        }

The call to memset() above initializes the entire struct to zero, any pointers are then turned into NULL.

Based

StackExchange Code Review Q#141396, answer score: 5