CGI script that reads or writes its height and weight configuration

Question

UPDATES:

I'm looking for an answer that can answer the questions listed in the post. Moreover, this answer will not only continue my way of thinking, but also give feedback based on it. It's just like a conversation.

For example: As to question 1.1. It's better to create andy_cfg to handle everything about andy.ini only if these considerations are met..., or you're doing good with this architecture so far.

Then give insight on each consideration.

There are two source files andy_web.c and andy_cfg.c.

andy_web.c manages things related to Web pages in the form of program, called andy_web.cgi and is invoked by lighttpd.

In brief, if get=setting is received, andy_web.cgi will take height and weight from andy's config file by calling functions in andy_cfg.c, and show them in pages; if post=setting is received, andy_web.cgi will get height and weight from lighttpd and set them to the config file also by calling functions in andy_cfg.c.

/*web_api.h*/

#ifndef WEB_API_H
#define WEB_API_H

#define MAX_NAME_LEN 128
#define MAX_VALUE_LEN 128

struct web_http_nv
{
    char name[MAX_NAME_LEN];
    char value[MAX_VALUE_LEN];
};

#define MAX_NAME_VALUE_LEN 512

typedef struct web_t web_t;
struct web_t
{
    struct web_http_nv nameval[MAX_NAME_VALUE_LEN]; /* (name, value) pair from web */
    int nv_ct; /* (name, value) pair count */
};

void web_get(web_t* web); /* get (name, value) pair from web */

#endif

/*andy_web.c*/

#include <stdio.h>
#include <string.h>
#include <stdlib.h>
#include "web_api.h"
#include "andy_cfg.h"

#define MIN_HEIGHT 150
#define MAX_HEIGHT 250

#define MIN_WEIGHT 50
#define MAX_WEIGHT 150

void printMsg(char *msg); //forward declaration

static andy_config_t* andy_config = NULL;
static web_t web = {0};

static void andy_web_page_show(void);
static int andy_web_page_setting_check_then_save(char Msg[], uint16_t MsgSize);

int main(void)
{
    andy_config = andy_config_init(180, 80);
    if (ANDY_CFG_FAILED == andy_config)
    {
        return -1;
    }
    
    web_get(&web);
    const uint8_t METHOD = web.nv_ct - 1;
    
    if (!strcmp(web.nameval[METHOD].name, "get"))
    {
        if (!strcmp(web.nameval[METHOD].value, "setting"))
        {
            andy_web_page_show();
        }
        
        goto DONE;
    }
    
    char Msg[100] = {0};
    if (!strcmp(web.nameval[METHOD].name, "post"))
    {
        if (!strcmp(web.nameval[METHOD].value, "setting"))
        {
            if (andy_web_page_setting_check_then_save(Msg, sizeof(Msg)) == -1)
            {
                goto SETTING_CHECK_ERROR;
            }
        }
        
        andy_web_page_show();
        
        goto DONE;
    }
    
SETTING_CHECK_ERROR:
    
    printMsg(Msg);

DONE:

    andy_config_close(andy_config);
    return 0;
}

static void andy_web_page_show(void)
{
    printf("Content-Type: text/html\n\n");
    printf("<html>\n");
    printf("<head>\n");
    printf("<meta http-equiv=\"Content-Type\" content=\"text/html;\" />\n");
    printf("<title>andy Setting</title>\n");
    printf("</head>\n");
    printf("<body>\n");
    printf("<fieldset>\n");
    printf("<legend>andy Setting</legend>\n");
    printf("<form  name=\"andyPage\" method=\"post\" action=\"/cgi-bin/andy_web.cgi\" >\n");
    printf("  <table id=\"table1\" border=\"0\" cellpadding=\"1\" cellspacing=\"1\">\n");
    
    uint16_t height = 0;
    if (andy_config_retrieve_height(andy_config, &height) != -1)
    {
        printf("    <tr>\n");
        printf("      <td height=\"20\" width=\"150\" style=\"padding-left:10px\">andy height</td>\n");
        printf("      <td height=\"20\" ><input type=\"text\" name=\"height\" style=\"width: 100;\" value=\"%d\" />(%d~%d)</td>\n", height, MIN_HEIGHT, MAX_HEIGHT);
        printf("    </tr>\n");
    }
    
    uint16_t weight = 0;
    if (andy_config_retrieve_weight(andy_config, &weight) != -1)
    {
        printf("    <tr>\n");
        printf("      <td height=\"20\" width=\"150\" style=\"padding-left:10px\">andy weight</td>\n");
        printf("      <td height=\"20\" ><input type=\"text\" name=\"weight\" style=\"width: 100;\" value=\"%d\" />(%d~%d)</td>\n", weight, MIN_WEIGHT, MAX_WEIGHT);
        printf("    </tr>\n");
    }
    
    printf("  </table>\n");
    printf("  <p align=\"center\">\n");
    printf("  <input type=\"submit\" value=\"   Update   \" />\n");
    printf("  <input type=\"hidden\" name=\"post\" value=\"setting\" />\n");
    printf("  </p>\n");
    printf("</form>\n");
    printf("</body>\n");
    printf("</html>\n");
}

static int andy_web_page_setting_check_then_save(char Msg[], uint16_t MsgSize)
{   
    int count = 0;
    for (; count < web.nv_ct-1; count++)
    {
        if (!strcmp(web.nameval[count].name, "height"))
        {
            uint16_t height = atoi(web.nameval[count].value);
            if ((height < MIN_HEIGHT) || (height >= MAX_HEIGHT))
            {
                snprintf(Msg, MsgSize, "height <%d> should be between %d and %d", height, MIN_HEIGHT, MAX_HEIGHT);
                return -1;
            }
            
            andy_config_set_height(andy_config, height);
        }
        else if (!strcmp(web.nameval[count].name, "weight"))
        {
            uint16_t weight = atoi(web.nameval[count].value);
            if ((weight < MIN_WEIGHT) || (weight >= MAX_WEIGHT))
            {
                snprintf(Msg, MsgSize, "weight <%d> should be between %d and %d", weight, MIN_WEIGHT, MAX_WEIGHT);
                return -1;
            }
            
            andy_config_set_weight(andy_config, weight);
        }
        else
        {

        }
    }

    andy_config_save(andy_config);
    return 0;
}

andy_cfg.c manages things related to the config file in the form of functions that are called by andy_web.c.

Function names beginning with iniparser, such as iniparser_load, are offered by iniparser which basically parses ini files and offer abilities to read/write text files in C level.

/*andy_cfg.h*/

#ifndef ANDY_CFG_H
#define ANDY_CFG_H

#include <stdint.h>

typedef struct andy_config_t andy_config_t;

#define ANDY_CFG_FAILED (andy_config_t*)NULL


andy_config_t* andy_config_init(uint16_t height, uint16_t weight);
void andy_config_close(andy_config_t* me);

void andy_config_save(andy_config_t* me);

void andy_config_set_height(andy_config_t* me, uint16_t height);
void andy_config_set_weight(andy_config_t* me, uint16_t weight);

int andy_config_retrieve_height(andy_config_t* me, uint16_t* height);
int andy_config_retrieve_weight(andy_config_t* me, uint16_t* weight);

#endif

/*andy_cfg.c*/

#include <stdbool.h>
#include "iniparser.h"
#include "andy_cfg.h"

#define ANDY_STARTUP_DIR "/mnt/jffs2/cfg/"
#define ANDY_RUNNING_DIR "/var/"
#define ANDY_CFG_FILE "andy.ini"

bool file_is_existing(const char* file_path); //forward declaration

andy_config_t* andy_config_init(uint16_t height, uint16_t weight)
{
    if (!file_is_existing(ANDY_STARTUP_DIR""ANDY_CFG_FILE))
    {   
        FILE* andy_startup_file = fopen(ANDY_STARTUP_DIR""ANDY_CFG_FILE, "w");
        if (NULL == andy_startup_file)
        {
            return ANDY_CFG_FAILED;
        }
        
        fprintf(andy_startup_file,
        "\n"
        "[Info]\n"
        "\n"
        "height = %d ; 150 ~ 250\n"
        "weight = %d ; 50 ~ 150\n"
        "\n", height, weight);
        
        fclose(andy_startup_file);
    }
    
    dictionary* andy_running_config = iniparser_load(ANDY_STARTUP_DIR""ANDY_CFG_FILE);
    if (NULL == andy_running_config)
    {
        return ANDY_CFG_FAILED;
    }

    return (andy_config_t*)andy_running_config;
}

void andy_config_close(andy_config_t* me)
{
    if (NULL != me)
    {
        andy_config_save(me);
        iniparser_freedict((dictionary*)me);
    }
}

void andy_config_save(andy_config_t* me)
{
    FILE* andy_startup_file = fopen(ANDY_STARTUP_DIR""ANDY_CFG_FILE, "w");
    iniparser_dump_ini((dictionary*)me, andy_startup_file);
    fclose(andy_startup_file);
}

void andy_config_set_height(andy_config_t* me, uint16_t height)
{   
    char str[10] = {0};
    snprintf(str, sizeof(str), "%d", height);
    iniparser_set((dictionary*)me, "Info:height", str);
}

void andy_config_set_weight(andy_config_t* me, uint16_t weight)
{   
    char str[10] = {0};
    snprintf(str, sizeof(str), "%d", weight);
    iniparser_set((dictionary*)me, "Info:weight", str);
}

int andy_config_retrieve_height(andy_config_t* me, uint16_t* height)
{
    int ret = iniparser_getint((dictionary*)me, "Info:height", -1);
    if (ret == -1)
    {
        return -1;
    }

    *height = (uint16_t)ret;
    return 0;
}

int andy_config_retrieve_weight(andy_config_t* me, uint16_t* weight)
{
    int ret = iniparser_getint((dictionary*)me, "Info:weight", -1);
    if (ret == -1)
    {
        return -1;
    }

    *weight = (uint16_t)ret;
    return 0;
}

Now that I've covered the code, I want to point out some of my perspective about the code.

andy_web.cgi should only deal with and aware of nothing else but Web matters.

1.1. Would it be a good choice for andy_web.cgi that creating a program andy_cfg that deals with everything about andy.ini and to retrieve/save data from/to it through IPC? If so, under what considerations you will take this approach?

Or, one can create a program cfg that handles config files not only for andy but everyone. However, this way will not go well because cfg will include so many header files such as andy_cfg.h, amy_cfg.h, benson_cfg.h, and end up with maintenance issues.

I've seen code somewhere that seems to be designed to solve this maintenance issues. Basically there's one callback function, and all of a sudden it's like the relationship is upside down between caller and callee. The code might look like

/*cfg.c*/

typedef void(*register_function_t)(void);

void cfg_register_set(char* function_name, register_function_t callback_function)
{
    /*Object which wants to use config feature 
    will call this function to register this feature*/
}

void cfg_init(void)
{
    /*After registration, all cfg files will be handled in some way.*/
}

1.2 How the above code works? Is there other common practices to deal with this kind of maintenance problem? Please tell me in detail.

andy_cfg.c seems to violate DRY (don't repeat yourself).

This way will end up with so many source files such like andy_cfg.c, amy_cfg.c, benson_cfg.c, etc. All of these codes have everything in common except for the function names that provide literally the same functionality. For example, amy_cfg.h will be like

/*amy_cfg.h*/

#ifndef AMY_CFG_H
#define AMY_CFG_H

#include <stdint.h>

typedef struct amy_config_t amy_config_t;

#define AMY_CFG_FAILED (amy_config_t*)NULL


amy_config_t* amy_config_init(uint16_t height, uint16_t weight);
void amy_config_close(amy_config_t* me);

void amy_config_save(amy_config_t* me);

void amy_config_set_height(amy_config_t* me, uint16_t height);
void amy_config_set_weight(amy_config_t* me, uint16_t weight);

int amy_config_retrieve_height(amy_config_t* me, uint16_t* height);
int amy_config_retrieve_weight(amy_config_t* me, uint16_t* weight);

#endif

/*amy_cfg.c*/

#include <stdbool.h>
#include "iniparser.h"
#include "amy_cfg.h"

#define AMY_STARTUP_DIR "/mnt/jffs2/cfg/"
#define AMY_RUNNING_DIR "/var/"
#define AMY_CFG_FILE "amy.ini"

bool file_is_existing(const char* file_path); //forward declaration

amy_config_t* amy_config_init(uint16_t height, uint16_t weight)
{
    if (!file_is_existing(AMY_STARTUP_DIR""AMY_CFG_FILE))
    {   
        FILE* amy_startup_file = fopen(AMY_STARTUP_DIR""AMY_CFG_FILE, "w");
        if (NULL == amy_startup_file)
        {
            return AMY_CFG_FAILED;
        }

        fprintf(amy_startup_file,
        "\n"
        "[Info]\n"
        "\n"
        "height = %d ; 150 ~ 250\n"
        "weight = %d ; 50 ~ 150\n"
        "\n", height, weight);

        fclose(amy_startup_file);
    }

    dictionary* amy_running_config = iniparser_load(AMY_STARTUP_DIR""AMY_CFG_FILE);
    if (NULL == amy_running_config)
    {
        return AMY_CFG_FAILED;
    }

    return (amy_config_t*)amy_running_config;
}

void amy_config_close(amy_config_t* me)
{
    if (NULL != me)
    {
        amy_config_save(me);
        iniparser_freedict((dictionary*)me);
    }
}

void amy_config_save(amy_config_t* me)
{
    FILE* amy_startup_file = fopen(AMY_STARTUP_DIR""AMY_CFG_FILE, "w");
    iniparser_dump_ini((dictionary*)me, amy_startup_file);
    fclose(amy_startup_file);
}

void amy_config_set_height(amy_config_t* me, uint16_t height)
{   
    char str[10] = {0};
    snprintf(str, sizeof(str), "%d", height);
    iniparser_set((dictionary*)me, "Info:height", str);
}

void amy_config_set_weight(amy_config_t* me, uint16_t weight)
{   
    char str[10] = {0};
    snprintf(str, sizeof(str), "%d", weight);
    iniparser_set((dictionary*)me, "Info:weight", str);
}

int amy_config_retrieve_height(amy_config_t* me, uint16_t* height)
{
    int ret = iniparser_getint((dictionary*)me, "Info:height", -1);
    if (ret == -1)
    {
        return -1;
    }

    *height = (uint16_t)ret;
    return 0;
}

int amy_config_retrieve_weight(amy_config_t* me, uint16_t* weight)
{
    int ret = iniparser_getint((dictionary*)me, "Info:weight", -1);
    if (ret == -1)
    {
        return -1;
    }

    *weight = (uint16_t)ret;
    return 0;
}

My instinct told me that I should write type-general functions for iniparser, so that I can reuse it. Unfortunately, easier said than done.

2.1 Is writing type-general functions for iniparser, in order to solve DRY problem, a good choice in C? Is there other common practices?

There once was another way that came into my mind, but I was not sure that it would be practicable.

What if I wrapped iniparser as a program and let other programmers to specify arguments on command line? In other words, one can create config file for everyone by typing iniparser someone.ini height=180 weight=80 on the command line, without having to create someone_cfg.c repeatedly.

Also, for the command line, adding new attribute is much easier than a function approach. For example, what if age attribute came around? For command-line approach, one just type iniparser someone.ini height=180 weight=80 age=32; for function approach, one has to add void someone_cfg_set_age(someone_cfg_t* me, uint8_t age);, to re-compile source file, and to re-link program.

All iniparser does is just to put arguments on the command line in a file. Simple and straightforward.

However, this approach not only shifts the learning curve of iniparser from the function level to the program level, but it is only applicable to programmers who know scripts. Not to mention that the program itself may have bugs!

2.2 Is this really a good approach? Is there other common practices?

andy_web.c should not know details about config file.

What if config files were handled in binary files instead of text files? The code would have to re-compile and re-link. Fortunately, this problem can be solved by creating a program andy_cfg that is described in 1.1.

I spent several hours for this post. Please share your comment in any way! Thank you.

How would andy_cfg.c, amy_cfg.c, benson_cfg.c, differ from each other? Sharing andy_cfg.h (specifically a definition of andy_config_t), and a hypothetical amy_cfg.h will seriously help to address your concerns. — vnp, Commented Jul 12, 2021 at 0:19
Where MAX_NAME_LEN, MAX_VALUE_LEN defined? Try compiling what is posted. You have a lot here with many questions (11+). Perhaps review a subset of code or focus the review? — chux, Commented Jul 14, 2021 at 7:51
@chux-ReinstateMonica I apologize for my impatience. Now the source code is able to compile except for functions file_is_existing to check whether file is existing or not , web_get to get (name, value) pair from web, and printMsg to print message on stdout. — Andy Lin, Commented Jul 14, 2021 at 8:29

G. Sliepen · Accepted Answer · 2021-07-16 11:00:28Z

Make your code more generic

What if it's not only Andy, Amy and Benson, but you want to store the height and weight of a thousand people? Maybe some share the same first name? You can imagine that this would result in a terrible mess. The first thing to do is to write your code in a generic way, so it can handle any number of persons without having to duplicate code in any way. Indeed, follow the DRY principle as much as possible.

Anything that currently has "andy" in its name should be replaced with something that is generic. That includes files like andy_web.c, and structures like andy_config_t. That means you have to find some way to pass the name "Andy" as a variable. For example:

struct person_config_t {
    const char *name;
    uint16_t height;
    uint16_t weight;
};

You also have to pass this in the HTML form. For example:

static void web_page_show(const char *name)
{
    printf("Content-Type: text/html\n\n");
    ...
    printf("  <input type=\"hidden\" name=\"name\" value=\"%s\"/>\n",
           html_quote(name));
    ...
}

This allows you to read back the name of the person in main(), and use that to open the right configuration file. Please make sure you sanitize the name before using it; someone could set the name field to "../../../etc/passwd" for example.

Use a proper database

INI files are great to store configuration, but that is about it. If you want to store and manipulate data for multiple people, you should start using a proper database. This might sound scary, but there are many simple, light-weight databases that you can use. The most well-known one is SQLite, and this sounds like a good fit for your project. With SQLite, there is no need to run a database server; everything is just stored in a single file, so in that respect it is not much different from an INI file.

Speaking of genericity, could you list the differences between function approach that you provided in your answer and program approach, and give reasons when a person should use which one? E.g. In ___ situation you should consider using function approach rather than program approach, because ___. I personally hope answer will answer the questions listed in post. — Andy Lin, Commented Jul 21, 2021 at 2:49

Edward · Accepted Answer · 2021-08-24 14:25:22Z

Here are a number of things that may help you improve your program.

Fix the bug

If the ini file does not exist, the andy_config_init() function will write a tag "wieght" to the file and because of the spelling error, the weight field will never be displayed. Computers are not very tolerant of spelling errors; you shouldn't be either, even in comments which have a number of errors such as "declartion" instead of "declaration" and in function names that have "retrive" instead of "retrieve". Fixing the first error makes your program run properly; fixing the other spelling errors will make it easier for others to read and understand your code.

Use a database

The other review has already addressed this, but using a database such as SQLite is a far better and more durable choice than the current implementation. Another possibility might be redis and hash sets.

Don't hardcode file names and directories

Instead of putting runtime data in an .ini file, use an .ini file for configuration such as the startup directory name and the name of the database.

Use `const` where practical

The MIN_WEIGHT and related constants don't have a type, but need to be able to fit into a uint16_t according to the logic of the code. Instead of writing them as macros, then, I'd suggest using const:

const uint16_t min_weight = 50;

Now it has a type and if the constant doesn't fit into a uint16_t, the compiler should warn you at compile time rather than manifesting some mysterious bug at runtime.

Hold the spaghetti

Code with convoluted control flow is often derisively called "spaghetti code." One sign of such code is the use of goto. The code currently has this rather tortured control flow:

    if (!strcmp(web.nameval[METHOD].name, "get"))
    {
        if (!strcmp(web.nameval[METHOD].value, "setting"))
        {
            andy_web_page_show();
        }
        
        goto DONE;
    }
    
    char Msg[100] = {0};
    if (!strcmp(web.nameval[METHOD].name, "post"))
    {
        if (!strcmp(web.nameval[METHOD].value, "setting"))
        {
            if (andy_web_page_setting_check_then_save(Msg, sizeof(Msg)) == -1)
            {
                goto SETTING_CHECK_ERROR;
            }
        }
        
        andy_web_page_show();
        
        goto DONE;
    }
    
SETTING_CHECK_ERROR:
    
    printMsg(Msg);

DONE:

    andy_config_close(andy_config);
    return 0;

There are a lot of problems with this, but the main one is that it's hard to follow. What should happen if "name" is neither "post" nor "get"? What actually happens is that printMsg() gets called with an empty message, but that doesn't really seem right. Also if "name" is "get", do we really need to check if it's also "post"? If it matches "get" but not "settings" we don't call andy_web_page_show() but if we match "post" but not "settings" it does. This seems very inconsistent and random. Here's how I would rewrite it:

First, notice that the nested if statements are really just checking for a particular name and value Let's isolate that to a function called matches_name_value()

bool matches_name_value(const struct web_http_nv *nv, const char* name, const char* value) {
    return strcmp(nv->name, name) == 0 && strcmp(nv->value, value) == 0;
}

Now we can use this and greatly simplify:

char Msg[100] = {0};
if (matches_name_value(&web.nameval[METHOD], "get", "setting")) {
    andy_web_page_show();
} else if (matches_name_value(&web.nameval[METHOD], "post", "setting")) {
    if (andy_web_page_setting_check_then_save(Msg, sizeof(Msg)) == -1) {
        printMsg(Msg);
    } else {
        andy_web_page_show();
    }
}
andy_config_close(andy_config);

Write better HTML

In the output, I'd strongly suggest adding a character encoding to be compliant with modern standards. Also, the code emits a <fieldset> and <legend> outside the <form> where they should instead be inside.

Consider separating CSS

Right now a lot of the bulk of the code output is in formatting which would probably be better suited to relegate to a separate css file. That makes the code cleaner and also makes it possible to customize the look of the output without modifying the CGI code.

Stack Exchange Network

CGI script that reads or writes its height and weight configuration

2 Answers 2

Make your code more generic

Use a proper database

Fix the bug

Use a database

Don't hardcode file names and directories

Use `const` where practical

Hold the spaghetti

Write better HTML

Consider separating CSS

Your Answer

Not the answer you're looking for? Browse other questions tagged
c
generics
dependency-injection
configuration
cgi
or ask your own question.

Hot Network Questions

CGI script that reads or writes its height and weight configuration

2 Answers 2

Make your code more generic

Use a proper database

Fix the bug

Use a database

Don't hardcode file names and directories

Use const where practical

Hold the spaghetti

Write better HTML

Consider separating CSS

Your Answer

Sign up or log in

Post as a guest

Not the answer you're looking for? Browse other questions tagged cgenericsdependency-injectionconfigurationcgi or ask your own question.

Related

Hot Network Questions

Use `const` where practical

Not the answer you're looking for? Browse other questions tagged
c
generics
dependency-injection
configuration
cgi
or ask your own question.