7
\$\begingroup\$

I'm looking at writing a simple proxy. At the moment I've got a logger, command line argument parse and simple daemon (it does nothing beyond logging).

Next I'll be looking at starting the network-related work, but before that I wanted to get a review to see if there's anything wrong with my basic approach at the moment.

I've spent most of my time recently working in Java/bash/PHP so want to know if there's anything about my approach that "smells" wrong.

proxy.cpp

#include <sys/types.h>
#include <sys/stat.h>
#include <unistd.h>

#include <boost/filesystem.hpp>

#include <commandLineOpts.hpp>
#include <loggerConfig.hpp>
#include <spdlog/spdlog.h>

namespace fs = boost::filesystem;

int main(int argc, char *argv[]) {
    CommandLineOpts commandLineOpts;
    commandLineOpts.parseOpts(argc, argv);

    if (commandLineOpts.contains(CommandLineOpts::Help)) {
        commandLineOpts.showHelp();
        return EXIT_SUCCESS;
    }

    std::string configFile = commandLineOpts.getConfig();
    fs::path absConfigPath = fs::absolute(fs::path(configFile));
    if (!exists(absConfigPath)) {
        std::cerr << "Configuration file " << absConfigPath << " does not exist." << std::endl << std::endl;
        commandLineOpts.showHelp();
        return EXIT_FAILURE;
    }

    pid_t pid, sid;    
    pid = fork();

    if (pid < 0) {
        std::cerr << "Failed to fork, error code [" << pid << "]. Exitting";
        return EXIT_FAILURE;
    } else if(pid > 0) {
        return EXIT_SUCCESS;
    }

    umask(0);

    /*
     * Create a new signature ID for the child
     */
    sid = setsid();
    if (sid < 0) {
        std::cerr << "Failed to setsid, error code [" << sid << "]. Exiting";
        return EXIT_FAILURE;
    }

    if ((chdir("/")) < 0) {
        std::cerr << "Failed to change directory to /. Exiting";
        return EXIT_FAILURE;
    }

    LoggerConfig logConfig(absConfigPath.string());
    logConfig.setup();
    auto logger = spdlog::get("async_file_logger");

    close(STDIN_FILENO);
    close(STDOUT_FILENO);
    close(STDERR_FILENO);

    while (true) {
        logger->debug() << "TODO:";
        sleep(60);
    }

    return EXIT_SUCCESS;
}

commandLineOpts.hpp

#ifndef COMMAND_LINE_OPTS_H
#define COMMAND_LINE_OPTS_H

#include <boost/program_options.hpp>

class CommandLineOpts {
    private:
        boost::program_options::variables_map commandLineMap;
        std::string appName;

    public:
        void parseOpts(int argc, char **argv);

        bool contains(std::string argName);
        void showHelp();
        std::string getConfig();

        static std::string Help;
        static std::string Config;
};

#endif

commandLineOpts.cpp

#include <boost/program_options.hpp>
#include <stdexcept>

#include <commandLineOpts.hpp>

namespace bpo = boost::program_options;

std::string CommandLineOpts::Help = "help";
std::string CommandLineOpts::Config = "config";

void CommandLineOpts::parseOpts(int argc, char **argv) {
    appName = argv[0];

    try {
        bpo::options_description desc("Allowed options");
        desc.add_options()
            ("help,h", "Display help")
            ("config,c", bpo::value<std::string>(), "Set config file");

        bpo::store(bpo::parse_command_line(argc, argv, desc), commandLineMap);
        bpo::notify(commandLineMap);
    } catch(std::exception& e) {
        throw new std::runtime_error(e.what());
    } catch(...) {
        throw new std::runtime_error("Failed to parse command line arguments, exception of unknown type!");
    }
}

bool CommandLineOpts::contains(std::string argName) {
    return commandLineMap.count(argName);
}

void CommandLineOpts::showHelp() {
    std::cout << "Usage " << appName << " [OPTION]" << std::endl
        << "-h, --help\t\tDisplay this usage" << std::endl
        << "-c, --config\t\tSpecify the configuration file to use" << std::endl;
}

std::string CommandLineOpts::getConfig() {
    if (contains(CommandLineOpts::Config)) {
        return commandLineMap[CommandLineOpts::Config].as<std::string>();
    } else {
        return "app.cfg";
    }
}

loggerConfig.hpp

#ifndef LOGGER_CONFIG_H
#define LOGGER_CONFIG_H

#include <map>
#include <string>

class LoggerConfig {

    private:
        std::string configFile;
        std::map<std::string, std::string> configMap;

        void loadConfig();
        void setDefaults();

        std::string get(std::string);

        std::string path(void);
    public:
        //! Constructor -- string path to the config file
        LoggerConfig(std::string cfg);

        void dump();

        void setup();

        static std::string LogPath;
        static std::string LogName;
        static std::string LogLevel;
        static std::string LogConfigName;
};

#endif

loggerConfig.cpp

#include <stdexcept>
#include <libconfig.h++>

#include <loggerConfig.hpp>
#include <spdlog/spdlog.h>

using namespace libconfig;

std::string LoggerConfig::LogConfigName = "log";
std::string LoggerConfig::LogPath = "LogPath";
std::string LoggerConfig::LogName = "LogName";
std::string LoggerConfig::LogLevel = "LogLevel";

LoggerConfig::LoggerConfig(std::string config) : configFile(config) {
    setDefaults();
    loadConfig();
}

void LoggerConfig::setDefaults() {
    configMap[LoggerConfig::LogPath] = "./log";
    configMap[LoggerConfig::LogName] = "proxy";
    configMap[LoggerConfig::LogLevel] = "info";
}

void LoggerConfig::loadConfig() {
    Config cfg;

    try {
        cfg.readFile(configFile.c_str());
    } catch (const FileIOException &fioex) {
        return;
    } catch (const ParseException &pex) {
        return;
    }

    const Setting &root = cfg.getRoot();
    try {
        const Setting &log = root[LoggerConfig::LogConfigName];
        std::string temp;

        try {
            log.lookupValue(LoggerConfig::LogPath.c_str(), temp);
            configMap[LoggerConfig::LogPath] = temp;
        } catch (const SettingNotFoundException &nfex) { }


        try {
            log.lookupValue(LoggerConfig::LogName.c_str(), temp);
            configMap[LoggerConfig::LogName] = temp;
        } catch (const SettingNotFoundException &nfex) { }


        try {
            log.lookupValue(LoggerConfig::LogLevel.c_str(), temp);
            configMap[LoggerConfig::LogLevel] = temp;
        } catch (const SettingNotFoundException &nfex) { }

    } catch (const SettingNotFoundException &nfex) {
        std::cerr << "Log config not found" << std::endl;
    }
}

std::string LoggerConfig::get(std::string cfg) {
    auto pos = configMap.find(cfg);

    if (pos == configMap.end()) {
        throw std::runtime_error("Logger config element [" + cfg + "] does not exist");
    } else {
        return pos->second;
    }
}

std::string LoggerConfig::path(void) {
    return get(LoggerConfig::LogPath) + "/" + get(LoggerConfig::LogName);
}

void LoggerConfig::setup() {

    try {        
        spdlog::set_async_mode(1048576);
        auto async_file = spdlog::daily_logger_mt("async_file_logger", path(), 23, 59, true);

        std::string logLevel = get(LoggerConfig::LogLevel);
        if (logLevel == "debug") {
            async_file->set_level(spdlog::level::debug);
        } else if (logLevel == "notice") {
            async_file->set_level(spdlog::level::notice);        
        } else if (logLevel == "warn") {
            async_file->set_level(spdlog::level::warn);        
        } else if (logLevel == "error") {        
            async_file->set_level(spdlog::level::err);
        } else {
            async_file->set_level(spdlog::level::info);        
        }
    } catch (const spdlog::spdlog_ex &ex) {
        throw std::runtime_error(ex.what());
    }
}

void LoggerConfig::dump() {
    std::cout << "LoggerConfig::dump " << std::endl;
    for (auto pos = configMap.cbegin() ; pos != configMap.cend() ; pos++) {
        std::cout << pos->first << "=" << pos->second << std::endl;
    }
}
\$\endgroup\$
  • \$\begingroup\$ I realize you're doing this as a learning exercise. But it is almost always better (simpler, less fragile) to write a "normal" program than daemonize it using a specialized external program like supervidor. They've worked out the nasty details so you don't have to rethink and retest it all. \$\endgroup\$ – Chris Johnson Sep 19 '15 at 21:59
  • \$\begingroup\$ Thanks, I was aware daemonize but will take a look at supervidor too \$\endgroup\$ – AJefferiss Sep 21 '15 at 7:39
  • \$\begingroup\$ You seem to have misspelt "session ID" in the comment Create a new signature ID for the child. \$\endgroup\$ – Toby Speight Jul 4 '18 at 7:49
6
\$\begingroup\$
  • It may be useful to use more specific error codes, such as EX_OSERR for a failed fork() call.

    In addition, your output statement can be improved. Since fork() updates errno on failure, replace your error output with just a call to std::perror() for a more accurate one.

    if (pid < 0) {
        std::perror("fork");
        return EX_OSERR;
    }
    
  • When passing a non-native type (such as std::string) to a function, it's preferred to pass by const& to avoid an extra copy:

    bool contains(std::string const& argName);
    

    bool CommandLineOpts::contains(std::string const& argName) {
        return commandLineMap.count(argName);
    }
    
  • Also regarding const: many of your member functions don't modify data members, yet they don't have const. This helps to prevent accidental modification of data members (causing a compiler error) while revealing more of the function's intent to others.

    std::string path(void) const;
    

    std::string LoggerConfig::path(void) const {
        return get(LoggerConfig::LogPath) + "/" + get(LoggerConfig::LogName);
    }
    
  • Unlike in C, it's unnecessary to specify void for functions that take no arguments. The compiler will already be aware of this.

  • In parseOpts(), it doesn't look like you should try to catch either an std::exception or ... and then throw either one as an std::runtime_error. The specific exception you may be looking for is std::logic_error. But this would depend on exactly which exception(s) can be thrown in this function.

  • In showHelp(), you don't need to do all that flushing with std::endl, which affects performance. Instead, output a "\n" for just a newline.

  • Since you're using C++11, you can use range-based for loops. For example, this can be applied to the loop in dump():

    for (auto& it : configMap) {
        std::cout << it.first << "=" << it.second << std::endl;
    }
    
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Excellent, thank you I was completely unaware of std::perror and will definitely make use of it in future. With regards to the flushing in showHelp() is it generally preferable to have std::endl flush at the end of the output? Or will a newline cause that? \$\endgroup\$ – AJefferiss Sep 21 '15 at 7:37
  • \$\begingroup\$ It can still be placed at the very end, if you really want to do a flush. \$\endgroup\$ – Jamal Sep 21 '15 at 7:44

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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