Raspberry PI switching from Soft Access Point to WIFI client

Question

I am relatively new to C language and Linux systems I would like some feedback/review. The main function of the code is to read the dhcpcd file looking for the 3 lines and then either adding/removing to the new file. It works and the Pi can switch from AP to client without a reboot, but I feel like it is more of a hack than a proper solution.

#define _GNU_SOURCE
#include <stdio.h>
#include <stdlib.h>
#include <string.h>

struct word {
    int linenumber;
    char *value;
};

struct words {
    int wordcount;
    struct word wordlist[3];
};

int main(void) {
    FILE *fp;
    FILE *temp;
    char filename[] = "/etc/dhcpcd.conf";
    char tempname[] = "/etc/temp.conf";
    char *line = NULL;
    size_t len = 0;
    ssize_t read;
    int linecount = 0;
    int index = 0;
    int shouldSkip = 0;
    struct words searchlist;
    struct word wordA;
    struct word wordB;
    struct word wordC;

    wordA.value = "interface wlan0";
    wordA.linenumber = -1;
    wordB.value = "static ip_address=192.168.4.1/24";
    wordA.linenumber = -1;
    wordC.value = "nohook wpa_supplicant";
    wordC.linenumber = -1;

    searchlist.wordcount = 3;
    searchlist.wordlist[0] = wordA;
    searchlist.wordlist[1] = wordB;
    searchlist.wordlist[2] = wordC;

    fp = fopen(filename, "r");
    temp = fopen(tempname, "w");

    if (fp == NULL || temp == NULL) {
        printf("Unable to open file!\n");
        exit(-1);
    }

    while ((read = getline(&line, &len, fp)) != -1) {
        struct word *xword = &searchlist.wordlist[index];
        if(strstr(line, xword->value) != NULL) {
            xword->linenumber = linecount;
            if(index > 0 && index != searchlist.wordcount - 1) {
                if ((linecount - searchlist.wordlist[index - 1].linenumber) != 1) {
                    for (int i = 0; i < searchlist.wordcount; i++) {
                        struct word *wd = &searchlist.wordlist[i];
                        wd->linenumber = -1;
                        index = 0;
                    }
                }
            }
            index = (index == (searchlist.wordcount - 1)) ? index : index + 1;
        }
        linecount++;
    }
    linecount = 0;
    //AP is on so toggle off
    if (fseek(fp, 0L, SEEK_SET) == 0) {
      while ((read = getline(&line, &len, fp)) != -1) {
        for(int i = 0; i < searchlist.wordcount; i++) {
          if(linecount == searchlist.wordlist[i].linenumber && strstr(line, searchlist.wordlist[i].value) != NULL) {
            shouldSkip = 1;
          }
        }

            if(!shouldSkip)
          fputs(line, temp);
        shouldSkip = 0;
            linecount++;
      }
    }

    if (index != searchlist.wordcount - 1) {
       for(int i = 0; i < searchlist.wordcount; i++) {
         fputs(searchlist.wordlist[i].value, temp);
         fputs("\n", temp);
       }
    }

    fclose(fp);
    fclose(temp);
    remove(filename);
    rename(tempname, filename);
    if(line)
     free(line);

    if (index == searchlist.wordcount - 1) {
      printf("Turning WIFI on\n");
    } else {
      printf("Turning Access Point on\n");
    }
    system("sudo service hostapd stop");
    system("sudo service dhcpcd restart");
    system("sudo service hostapd start");

    exit(0);
}

Community · Accepted Answer · 2020-06-10 13:24:26Z

A few minor remarks:

All pointers to string literals should be const qualified to prevent accidental bugs. That is const char*.
if(line) free(line); is pointless, just call free(line). It is well-defined to call free with a null pointer as parameter, in which case it will do nothing. This is handy when you know that all your pointers that may point at stuff that needs to be freed were originally initialized to point at NULL.

(And similarly, it is good practice to assign the pointer to NULL after calling free(), if you plan to re-use that same pointer again.)
exit(0); at the end of main is pointless and potentially confusing. You are using C99 so you actually don't need to write anything at all there. Not writing anything in C99 is identical to writing return 0;, which in turn is the same thing as calling exit(0);.

pacmaninbw · Accepted Answer · 2020-02-28 14:11:30Z

1

There are a few compiler errors that may not have show up for you, the first major compiler problem is that read is a basic function in C input and output so the variable declaration ssize_t read; should be changed to something like size_t readsize. It might be good if you compiled with the -w flags to add warnings to the compiler error messages.

The second problem I see is that you might have compiled this with a C++ compiler rather than a C compiler, a strict C compiler will not recognize the function readline() in a strict C compiler fgets() should be used instead. You will have to make the variable line an array of characters.

char line[BUFSIZ];

The symbolic constant BUFSIZ is defined in the include file stdio.h.

    fgets(line, BUFSIZ, fp);

Overall it would be better if main() was a short function that called other functions, it is currently too complex (does too much).

edited Feb 28, 2020 at 14:11

answered Feb 27, 2020 at 19:02

pacmaninbw♦

26.2k13 gold badges47 silver badges114 bronze badges

\$\begingroup\$ Thank you for the feedback. Good catch on the read variable naming. Using fgets method, would I have to parse for ‘\n’ to get track the line count? \$\endgroup\$

RyxMontar
– RyxMontar

2020-02-27 23:44:47 +00:00
Commented Feb 27, 2020 at 23:44
\$\begingroup\$ No, fgets gets only one line at a time. If it is still there and you want to remove it just one of the reverse string searches to start from the back rather than the front. \$\endgroup\$

pacmaninbw
– pacmaninbw ♦

2020-02-28 05:25:02 +00:00
Commented Feb 28, 2020 at 5:25
\$\begingroup\$ read is not standard, but then on the other hand neither is getline. If one of them is available, the other is likely as well. Similarly, there is nothing called BUFSIZE in standard C. \$\endgroup\$

Lundin
– Lundin

2020-02-28 13:56:30 +00:00
Commented Feb 28, 2020 at 13:56
\$\begingroup\$ @Lundin My mistake it should be BUFSIZ \$\endgroup\$

pacmaninbw
– pacmaninbw ♦

2020-02-28 14:11:07 +00:00
Commented Feb 28, 2020 at 14:11

Add a comment |

Toby Speight · Accepted Answer · 2020-03-02 09:39:58Z

fclose(fp);
fclose(temp);

Ignoring the return value from these (specifically the latter) is not a good idea. Closing a buffered file stream is when the last of the data are written - we want to know if that fails.

remove(filename);
rename(tempname, filename);

Again, return value ignored when it's actually quite important.

if(line)
 free(line);

The test is unnecessary, as free() already performs the same test.

system("sudo service hostapd stop");
system("sudo service dhcpcd restart");
system("sudo service hostapd start");

More return values ignored when they shouldn't be.

Stack Exchange Network

Raspberry PI switching from Soft Access Point to WIFI client

3 Answers 3

You must log in to answer this question.

Hot Network Questions

Raspberry PI switching from Soft Access Point to WIFI client

3 Answers 3

You must log in to answer this question.

Related

Hot Network Questions