Arch Linux

T_baggins

Member

Registered: 2013-12-14

Posts: 3

Code review request before AUR upload; KISS C based Nvidia fan control

source code

About a year ago I had a fan that would toggle between 0RPM and 35RPM. It infuriated me. At the time I only found solutions that were dependent on Xorg Server. This eventually led me to the multitude of python based nvml solutions. I hate resource waste(python). So I quickly implemented a C based solution and over the last year have tinkered with it to bring it to its current state. I have 0 formal education in C(I just finished C Programming Language Second Edition). So I would like to request a once over on my genuine attempt to make something in a C way that others may find useful. Don't hesitate to be harsh.

loqs

Member

Registered: 2014-03-06

Posts: 18,645

Re: Code review request before AUR upload; KISS C based Nvidia fan control

Few minor changes I would suggest for the first half of the Makefile.  Use the CFLAGS environment variable if set.  Keep with convention $(CC) is the compiler executable without arguments.  Use a distinct target name from that of the binary.  make knows how to build an object file from a C source file with the same name so use that built in rule.  Use LDFLAGS when linking.

diff --git a/Makefile b/Makefile
index 8d58c04..c63024b 100644
--- a/Makefile
+++ b/Makefile
@@ -1,17 +1,15 @@
 DEBUG ?= 0
 
-CFLAGS = -Wall -g
+CFLAGS ?= -Wall -g
 
 ifeq ($(DEBUG), 1)
     CFLAGS += -DDEBUG
 endif
 
-CC=gcc $(CFLAGS)
+all: fanController-bin
 
-all: fanController
-
-fanController: fanController.c
-	$(CC) -o fanController fanController.c -lnvidia-ml 
+fanController-bin: fanController.o
+	$(CC) $(LDFLAGS) -o fanController fanController.o -lnvidia-ml
 
 install: fanController
 	sudo mv ./fanController /opt/fanController

Still todo in the Makefile,  use $DESTDIR,  remove use of sudo,  use install instead of cp, possibly use pkgconf.  I do not have the time to review the C.  From a very superficial glance you have the configuration hard coded in the C source which means the package must be rebuilt after configuration.

T_baggins

Member

Registered: 2013-12-14

Posts: 3

Re: Code review request before AUR upload; KISS C based Nvidia fan control

Use the CFLAGS environment variable if set.
Keep with convention $(CC) is the compiler executable without arguments.
Use a distinct target name from that of the binary.
make knows how to build an object file from a C source file with the same name so use that built in rule.
Use LDFLAGS when linking.
use $DESTDIR
remove use of sudo
use install instead of cp

Implemented.

possibly use pkgconf.

nvidia-ml does not provide a .pc

configuration hard coded in the C source which means the package must be rebuilt after configuration.

I struggled to find a config parser that wasn't overly complex for my use case of 2-4 key:value pairs.

I plan to use a TOML style config file and implement a function to import FanTargets and TempTargets, possibly also a PollingInterval and TempThreshold.

If you know of a simple library or implementation I could reference, or if I'm reinventing the wheel, please let me know.

loqs

Member

Registered: 2014-03-06

Posts: 18,645

Re: Code review request before AUR upload; KISS C based Nvidia fan control

LDFLAGS ?= -lnvidia-ml

I would suggest moving that to LIBS othwise if LDFLAGS environment variable is set but does not contain '-lnvidia-ml' the link will fail.

Two things picked up by `-Wextra`:

fanController.c: In function ‘runTimeSanity’:
fanController.c:68:22: warning: comparison of unsigned expression in ‘< 0’ is always false [-Wtype-limits]
   68 |   if (TempTargets[0] < 0) {
      |                      ^
fanController.c:76:21: warning: comparison of unsigned expression in ‘< 0’ is always false [-Wtype-limits]
   76 |   if (FanTargets[0] < 0) {
      | 

One by `-Wpedantic`

fanController.c:34:30: warning: ISO C does not permit named variadic macros [-Wvariadic-macros]
   34 | #define DEBUG_PRINT(fmt, args...)                                              \
      |

Which can be fixed by switching to __VA_ARGS__ (I also removed the noop implementation leaving it as expanding to nothing)

--- a/fanController.c
+++ b/fanController.c
@@ -28,12 +28,10 @@
 #include <unistd.h>
 
 #ifdef DEBUG
-#define DEBUG_PRINT(fmt, args...)                                              \
-  fprintf(stderr, "DEBUG: %s:%d: " fmt, __FILE__, __LINE__, ##args)
+#define DEBUG_PRINT(fmt, ...) \
+  fprintf(stderr, "DEBUG: %s:%d: " fmt, __FILE__, __LINE__, ##__VA_ARGS__)
 #else
-#define DEBUG_PRINT(fmt, args...)                                              \
-  do {                                                                         \
-  } while (0)
+#define DEBUG_PRINT(fmt, ...)
 #endif
 
 #define TEMP_THRESHOLD 2 // Degree celcius before action

T_baggins

Member

Registered: 2013-12-14

Posts: 3

Re: Code review request before AUR upload; KISS C based Nvidia fan control

-lnvidia-ml
-Wextra
-Wpedantic

fixed