Code Review Asked by NNNComplex on November 30, 2020
I’m new to C and programming in general (some minimal prior exposure to JS and python). This is also my first time working with shell commands, scripts, and cron. This program works on my macOS and Lubuntu running on a VM (so long as you run as root).
Using this program, you can set times throughout the day when your wifi turns off automatically. When your wifi is off, you can still manually turn on wifi for urgent situations for a short time (default is 30 seconds) before the program automatically turns off the wifi again.
The code is separated into 2 C programs and 1 shell script.
checkon.sh (runs in bg and turns off wifi after a short time if it detects it was turned on)
#!/bin/bash
# Check OS
if [[ "$OSTYPE" == "linux-gnu"* ]]; then
# If wifi on for ubuntu, keyword is UP
wifion=UP
elif [[ "$OSTYPE" == "darwin"* ]]; then
# Name of Wi-Fi device is en0
device=en0
# If wifi on for osx, keyword is active
wifion=active
fi
# Check status of wi-fi device (is it active or inactive?)
status() {
if [[ "$OSTYPE" == "linux-gnu"* ]]; then
ip l show enp0s3 | awk '/state/{print $9}'
elif [[ "$OSTYPE" == "darwin"* ]]; then
/sbin/ifconfig $device | awk '/status:/{print $2}'
fi
}
# If wifi is on, turn off after X seconds
isactive() {
sleep 30
if [[ "$OSTYPE" == "linux-gnu"* ]]; then
nmcli networking off
elif [[ "$OSTYPE" == "darwin"* ]]; then
/usr/sbin/networksetup -setairportpower $device off
fi
}
# Every 5 seconds, check if wifi is active or inactive
while :; do
if [[ "$(status)" == "$wifion" ]]
then isactive
fi
sleep 5
done
setcheck.c (cron will launch this program)
#include <stdio.h> // For strerror()
#include <stdlib.h> // For exit() and system()
#include <string.h> // For strcmp()
#include <unistd.h> // For fork(), getcwd()
#include <errno.h> // For errno
#include <time.h> // For time()
// Determine os
#if __APPLE__
#define PLATFORM_NAME "apple"
#elif __linux__
#define PLATFORM_NAME "linux"
#endif
#define MAXLINE 1000
// 1) If I don't assign macro to a variable, compiler will warn that code
// strcmp(PLATFORM_NAME, "apple") will result in unreachable code.
// Need to determine condition at run-time vs compile-time.
// 2) Need `static` to avoid -Wmissing-variable-declaration warning
static char os[MAXLINE];
int startcheck(void);
int endcheck(void);
int pmessages(char *message);
int turnonwifi(void);
int turnoffwifi(void);
// Cron calls main(), telling it to either turn on wifi (normal state)
// or turn off wifi (run script)
int main(int argc, char *argv[]) {
strcpy(os, PLATFORM_NAME);
if (argc == 1) {
printf("Include argument 'wifion' or 'wifioff'n");
return 1;
}
// Turn off or on the background script
if (!strcmp(argv[1], "wifion")) {
pmessages("Turning on wifi");
turnonwifi();
// Run program shuts down checkon script
if (endcheck())
return 1;
} else if (!strcmp(argv[1], "wifioff")) {
pmessages("Turning off wifi");
turnoffwifi();
if(startcheck())
return 1;
} else {
pmessages("Error: Argument must be 'wifion' or 'wifioff': %sn");
}
return 0;
}
// Launch checkon script
int startcheck(void) {
char cwd[MAXLINE];
getcwd(cwd, MAXLINE);
char *checkon_path2 = "/checkon.sh";
char *checkon_path = strcat(cwd, checkon_path2);
char *checkon_arg0 = "checkon.sh";
// Check if checkon.sh already exists, if so, print error and exit
// Need to use '-f' and include '.sh' in search for osx
int isrunning = 1;
if (!strcmp(os, "apple")) {
isrunning = system("/usr/bin/pgrep -f checkon.sh >>/dev/null 2>>/dev/null");
} else if (!strcmp(os, "linux")) {
// NTD: Why does system() return 0 when '-f' is used in
// ubuntu when process doesn't exist?
isrunning = system("/usr/bin/pgrep checkon >>/dev/null 2>>/dev/null");
} else {
printf("Warning: cannot determine if checkon.sh is runningn");
}
if(isrunning == 0) {
pmessages("Error: checkon.sh already exists");
exit(1);
}
// Fork creates a child process that is identical except it returns
// 0 for the child and the pid of the child for the parent process
int pid = fork();
// We fork so that child runs in background when parent exits
if (pid == 0) {
setsid();
execl(checkon_path, checkon_arg0, (char *) NULL);
if (errno) {
printf("errno %d: %sn", errno, strerror(errno));
return 1;
}
} else {
exit(0);
}
return 0;
}
// Find checkon script and kill it
int endcheck(void) {
// pgrep -f finds name of script and kill will end it.
// Redirect output otherwise if checkon.sh doesn't exist,
// command will print error to terminal
if (!strcmp(os, "apple")) {
system("/bin/kill $(/usr/bin/pgrep -f checkon.sh) >>/dev/null 2>>/dev/null");
} else if (!strcmp(os, "linux")) {
system("/usr/bin/pkill checkon >>/dev/null 2>>/dev/null");
}
return 0;
}
// Print messages
int pmessages(char *pmessage) {
time_t current_time = time(NULL);
char *c_time_string = ctime(¤t_time);
if (pmessage != NULL) {
printf("%s: %s", pmessage, c_time_string);
return 0;
} else {
printf("pmessages error: no message: %sn", c_time_string);
return 1;
}
}
// Turn on wifi
int turnonwifi() {
// Include full path b/c cron sets a different environment
// than shell, meaning PATH variable is different
if (!strcmp(os, "apple")) {
system("/usr/sbin/networksetup -setairportpower en0 on");
} else if (!strcmp(os, "linux")) {
system("nmcli networking on");
}
return 0;
}
// Turn off wifi
int turnoffwifi() {
// Include full path b/c cron sets a different environment
// than shell, meaning PATH variable is different
if (!strcmp(os, "apple")) {
system("/usr/sbin/networksetup -setairportpower en0 off");
} else if (!strcmp(os, "linux")) {
system("nmcli networking off");
}
return 0;
}
swifi.c (user interacts with this program to set cron jobs)
#include <stdio.h> // For printf()
#include <stdlib.h> // For system()
#include <string.h> // For strcmp()
#include <inttypes.h> // For strtol()
#include <unistd.h> // For getopt() and getcwd()
#include <errno.h> // For errno
#include <time.h> // For time(), ctime(), etc.
// Determine os
#if __APPLE__
#define PLATFORM_NAME "apple"
#elif __linux__
#define PLATFORM_NAME "linux"
#endif
#define MAXLINE 1000
struct hourmin {
int minute;
int hour;
};
// Need static keyword to avoid -Wmissing-variable-declaration warning
static char cwd[MAXLINE];
static char os[MAXLINE];
static int addflag, rmvflag;
// Default: Be on the whole day
static struct hourmin starttime = { 1, 0 };
static struct hourmin endtime = { 0, 0 };
void getarg(int argc, char **argv);
int addcron(struct hourmin *starttime, struct hourmin *endtime);
int rmvcron(struct hourmin *starttime, struct hourmin *endtime);
int clearcron(void);
int turnoffwifi(void);
int turnonwifi(void);
int gethourmin(char *strtime, struct hourmin *);
int istimebetween(void);
// Takes 3 arguments: 1) add or rmv, 2) start time (-s) and 3) end time (-e)
int main(int argc, char *argv[]) {
getcwd(cwd, MAXLINE);
strcpy(os, PLATFORM_NAME);
if (argc == 1) {
printf("Error: Provide at least one argument 'add' or 'rmv'n");
return 1;
}
getarg(argc, argv);
if (addflag) {
addcron(&starttime, &endtime);
if(istimebetween())
turnoffwifi();
printf("Wifi set to turn off at %d:%02dn", starttime.hour, starttime.minute);
printf("Wifi set to turn on at %d:%02dn", endtime.hour, endtime.minute);
} else if (rmvflag) {
rmvcron(&starttime, &endtime);
if(istimebetween())
turnonwifi();
printf("Timer removed. Start: %d:%02d End: %d:%02dn", starttime.hour, starttime.minute, endtime.hour, endtime.minute);
}
return 0;
}
// Parse arguments using getopt(). Need a separate function
// to handle GNU / Linux case and OSX / BSD case.
void getarg(int argc, char **argv) {
int ch; // getopt man page uses "ch"
addflag = rmvflag = 0;
if (!strcmp(os, "apple")) {
while (optind < argc) {
if ((ch = getopt(argc, argv, "s:e:")) != -1) {
switch (ch) {
case 's':
gethourmin(optarg, &starttime);
break;
case 'e':
gethourmin(optarg, &endtime);
break;
default:
break;
}
} else if (!strcmp(argv[optind], "add")) {
addflag = 1;
optind++;
} else if (!strcmp(argv[optind], "rmv")) {
rmvflag = 1;
optind++;
} else if (!strcmp(argv[optind], "clear")) {
clearcron();
optind++;
} else {
printf("Error: Unrecognized argument.n");
exit(1);
}
}
} else if (!strcmp(os, "linux")) {
while ((ch = getopt(argc, argv, "s:e:")) != -1) {
switch (ch) {
case 's':
gethourmin(optarg, &starttime);
break;
case 'e':
gethourmin(optarg, &endtime);
break;
default:
break;
}
}
int index;
for (index = optind; index < argc; index++) {
if (!strcmp(argv[optind], "add")) {
addflag = 1;
} else if (!strcmp(argv[optind], "rmv")) {
rmvflag = 1;
} else if (!strcmp(argv[optind], "clear")) {
clearcron();
} else {
printf("Error: Unrecognized argument.n");
exit(1);
}
}
}
}
// Adds cronjob to crontab
int addcron(struct hourmin *start_time, struct hourmin *end_time) {
char cron_wifioff[MAXLINE];
char cron_wifion[MAXLINE];
// NTD: Is there a cleaner way of preparing these commands?
sprintf(cron_wifioff, "(crontab -l 2>>/dev/null; echo '%d %d * * * %s/setcheck.out wifioff >>/dev/null 2>>/dev/null') | sort - | uniq - | crontab -", start_time->minute, start_time->hour, cwd);
sprintf(cron_wifion, "(crontab -l 2>>/dev/null; echo '%d %d * * * %s/setcheck.out wifion >>/dev/null 2>>/dev/null') | sort - | uniq - | crontab -", end_time->minute, end_time->hour, cwd);
system(cron_wifioff);
system(cron_wifion);
return 0;
}
// Removes cronjobs from crontab
int rmvcron(struct hourmin *start_time, struct hourmin *end_time) {
// rcron stands for remove cron
char rcron_wifioff[MAXLINE];
char rcron_wifion[MAXLINE];
// NTD: Is there a cleaner way of preparing these commands?
sprintf(rcron_wifioff, "(crontab -l | sort - | uniq - | grep -v '%d %d \* \* \* %s/setcheck.out wifioff >>/dev/null 2>>/dev/null') | crontab -", start_time->minute, start_time->hour, cwd);
sprintf(rcron_wifion, "(crontab -l | sort - | uniq - | grep -v '%d %d \* \* \* %s/setcheck.out wifion >>/dev/null 2>>/dev/null') | crontab -", end_time->minute, end_time->hour, cwd);
system(rcron_wifioff);
system(rcron_wifion);
return 0;
}
// Clears crontab and restarts wifi
int clearcron(void) {
system("crontab -r");
turnonwifi();
return 0;
}
int turnoffwifi(void) {
char wificmd[MAXLINE];
sprintf(wificmd, "%s/setcheck.out wifioff >>/dev/null 2>>/dev/null", cwd);
system(wificmd);
return 0;
}
int turnonwifi(void) {
char wificmd[MAXLINE];
sprintf(wificmd, "%s/setcheck.out wifion >>/dev/null 2>>/dev/null", cwd);
system(wificmd);
return 0;
}
// Get current time and compare if it is in between
// NTD: Not sure what to do if starttime = endtime. cronjob race condition?
int istimebetween(void) {
// Get current time into int format (tm struc that contains int members)
time_t current_time = time(NULL);
struct tm *current_tm = localtime(¤t_time);
// Rename for ease
int chour = current_tm->tm_hour;
int cmin = current_tm->tm_min;
int shour = starttime.hour;
int smin = starttime.minute;
int ehour = endtime.hour;
int emin = endtime.minute;
// If endtime > starttime, check if current time is in between
// If endtime < starttime, check if current time is NOT in between
int checkbetween = 0;
if ((ehour > shour) || (ehour == shour && emin > smin))
checkbetween = 1;
// Compare current time to starttime and endtime
int isbetween = 0;
switch (checkbetween) {
case 0:
// In case 0, endtime is "earlier" than starttime, so
// we check endtime < currenttime < starttime. 4 possibilities:
// 1) endhour < currenthour < starthour
// 2) endhour = currenthour < starthour; endmin < currentmin
// 3) endhour < currenthour = starthour; currentmin < startmin
// 4) endhour = currenthour = starthour; endmin < cmin < startmin
if ((chour > ehour && chour < shour)
|| (chour == ehour && cmin > emin && chour < shour)
|| (chour == shour && cmin < smin && chour > ehour)
|| (chour == ehour && chour == shour
&& cmin > emin && cmin < smin))
isbetween = 1;
break;
case 1:
// In case 1, end time is "later" than starttime, so
// we check starttime < currenttime < endtime. Inverse of above.
if ((chour > shour && chour < ehour)
|| (chour == shour && cmin > smin && chour < ehour)
|| (chour == ehour && cmin < emin && chour > shour)
|| (chour == shour && chour == ehour
&& cmin > smin && cmin < emin))
isbetween = 1;
break;
}
return (checkbetween == isbetween);
}
// Converts string with 'HHMM' format into struct hourmin
int gethourmin(char *strtime, struct hourmin *hmtime) {
// Arrays are size 3 because HH and MM can be at most 2 digits + ''
char hour[3] = { '0', '', '' };
char minute[3] = { '0', '', '' };
// Takes 'HHMM' string and separates into minutes and hours strings
switch (strlen(strtime)) {
case 1: case 2:
strcpy(hour, strtime);
break;
case 3:
hour[0] = *strtime++;
hour[1] = '';
strcpy(minute, strtime);
break;
case 4:
hour[0] = *strtime++;
hour[1] = *strtime++;
hour[2] = '';
strcpy(minute, strtime);
break;
default:
printf("Error: Bad time arguments. Must be of the form H, HH, HMM, or HHMM.n");
exit(1);
}
hmtime->hour = (int) strtol(hour, NULL, 10);
hmtime->minute = (int) strtol(minute, NULL, 10);
// Checks that mins and hours are legal values. First errno occurs if
// either strtol() above fail (if str can't be converted to number).
if (errno) {
printf("Error: Bad time argument. Must provide a number.n");
printf("errno: %sn", strerror(errno));
exit(1);
}
if (hmtime->hour > 23 || hmtime->hour < 0) {
printf("Error: Bad time argument. Hour must be between 0-23.n");
exit(1);
}
if (hmtime->minute > 59 || hmtime->minute <0) {
printf("Error: Bad time argument. Minute must be between 0-59.n");
exit(1);
}
return 0;
}
Remarks / Questions:
system()
is bad practice for reasons of security, portability, and performance (source). However, it wasn’t obvious to me whether reimplementing commands like pgrep
and crontab
using lower-level library and system calls were worthwhile in this case (or whether that is even the right approach).nmcli
command for reasons I don’t quite understand. I found others with similar issues (source, source, source), but none of the suggestions helped me fix the problem.setcheck.c
, rename as setcheck.out
, then compile swifi.c
, but that seems like a lot of manual work. I’m aware of Makefile
/ make
but I’m not familiar with them and not sure if they would be overkill in a small project like this.We can use plain POSIX /bin/sh
. The only Bashism I see is
if [[ "$OSTYPE" == "linux-gnu"* ]]; then
elif [[ "$OSTYPE" == "darwin"* ]]; then
fi
This is in any case more naturally expressed as
case "$(uname -s)" in
Linux)
;;
Darwin) # fill in actual value here
;;
esac
It would be good for the first of these to error-exit the script if there's no match.
Given that the implementations of the functions are generally completely different for each platform, I'd restructure to define them independently for each platform:
case "$(uname -s)" in
Linux)
status() { ... }
isactive() { ... }
;;
Darwin)
status() { ... }
isactive() { ... }
;;
esac
If we do this, we shouldn't need the $wifion
variable - just arrange for status
to exit with the appropriate return value:
status() {
ip link show enp0s3 | sed 1q | grep -qF 'state UP'
}
In the C code, I see no point creating PLATFORM_NAME
macro and the os
string, that we then only ever use to compare against fixed values. Why not simply use the existing boolean macros? E.g. instead of
if (!strcmp(os, "apple")) { system("/bin/kill $(/usr/bin/pgrep -f checkon.sh) >>/dev/null 2>>/dev/null"); } else if (!strcmp(os, "linux")) { system("/usr/bin/pkill checkon >>/dev/null 2>>/dev/null"); }
We could just
#if __APPLE__
system("/bin/kill $(/usr/bin/pgrep -f checkon.sh) >>/dev/null 2>>/dev/null");
#elif __linux__
system("/usr/bin/pkill checkon >>/dev/null 2>>/dev/null");
#else
#error This platform not yet supported
#endif
Some minor nits:
is
and str
for future library functions. So it's a bad idea to use such names in your code.getarg()
function is very duplicated - only a couple of lines need to vary between the platforms.fprintf(stderr, ...)
. I'm pleased to see a non-zero exit in the error case, and recommend using the standard EXIT_FAILURE
macro there.Answered by Toby Speight on November 30, 2020
I will offer some suggestions.
Please note I have no experience with Mac OS, but I believe most of it will still apply.
Using system
is not particularly bad in your case since you use static arguments in those calls excluding the possibility of command injection.
However, since your first C program is almost completely made up of system
calls, you should consider converting it to BASH script as well, since those work a lot better for "command glue" than C binaries.
In your C binaries you should not check the OS type at runtime with strcmp
.
This both wastes processing time, and makes your binary larger because it contains extra code that will never be used (unless you turn optimization high enough to remove it automatically).
Consider using conditional compilation instead.
If you are going to work with C on Linux and Mac, you should learn GNU Make, at least its basics.
Even a single file C program can benefit from a Makefile, as it will save you typing a long compiler command line.
Also, you can have one Makefile for both of your tools since they are part of the same package.
It is a very useful automation system any developer should have in their arsenal.
If you really intend to distribute your program for Linux, you will probably want to learn the basics of packaging deb or rpm.
Consider using better IPC.
Searching for background process with pgrep
every time is both inefficient and cumbersome.
Consider instead having the process create a "lock" or "flag" file with a fixed name in a fixed path while it is running and delete it on exit.
You can then easily and quickly test for this file both from BASH and C.
Also, instead of forcefully killing the processes use signals.
Finally, if you are going to communicate between C programs, such as a daemon and a client (for sending commands to the daemon), consider using sockets.
There is even a way use them from BASH, though it is less convenient and more limited.
I don't have suggestions for fixing your WiFi control issues, and I think that question is more suitable for stackoverflow than this site, but you should consider one thing:
Perhaps refactoring your application in to a system service will solve that problem, and make it more streamlined and efficient.
Answered by Lev M. on November 30, 2020
Get help from others!
Recent Questions
Recent Answers
© 2024 TransWikia.com. All rights reserved. Sites we Love: PCI Database, UKBizDB, Menu Kuliner, Sharing RPP