Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion Android.bp
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,10 @@ cc_binary {
name: "tqftpserv",
vendor: true,
srcs: [
"config.c",
"logging.c",
"tqftpserv.c",
"translate.c",
],
shared_libs: ["libqrtr"],
shared_libs: ["libqrtr", "libconfig"],
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the last change in the series. In patch 3 Dmitry suggested that you should use libconfig instead, you said it doesn't work on Android, but here you're adding a dependency on it.

So, commit message doesn't match content, and if this is necessary it clearly can't be the last change in the series - as it would break the build of previous commits.

}
296 changes: 296 additions & 0 deletions config.c
Original file line number Diff line number Diff line change
@@ -0,0 +1,296 @@
// SPDX-License-Identifier: BSD-3-Clause
/*
* Copyright (c) Qualcomm Technologies, Inc. and/or its subsidiaries.
*/

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <getopt.h>
#include <errno.h>
#include <unistd.h>
#include <libconfig.h>
#include "config.h"
#include "logging.h"

/* Global configuration instance */
struct tqftp_config tqftp_config;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this a global variable?


/**
* tqftp_config_init_defaults() - Initialize configuration with default values
* @config: Configuration structure to initialize
*/
void tqftp_config_init_defaults(struct tqftp_config *config)
{
/* Initialize path configuration */
strncpy(config->readonly_path, DEFAULT_READONLY_PATH, sizeof(config->readonly_path) - 1);
strncpy(config->readwrite_path, DEFAULT_READWRITE_PATH, sizeof(config->readwrite_path) - 1);
strncpy(config->firmware_base, DEFAULT_FIRMWARE_BASE, sizeof(config->firmware_base) - 1);
strncpy(config->updates_dir, DEFAULT_UPDATES_DIR, sizeof(config->updates_dir) - 1);
strncpy(config->temp_dir, DEFAULT_TEMP_DIR, sizeof(config->temp_dir) - 1);
strncpy(config->config_file, DEFAULT_CONFIG_FILE, sizeof(config->config_file) - 1);

/* Null terminate all strings */
config->readonly_path[sizeof(config->readonly_path) - 1] = '\0';
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there no version of strlcpy or similar that you can use?

If not, write your safe_strcpy() function instead of writing strncpy() + \0 all over the place.

config->readwrite_path[sizeof(config->readwrite_path) - 1] = '\0';
config->firmware_base[sizeof(config->firmware_base) - 1] = '\0';
config->updates_dir[sizeof(config->updates_dir) - 1] = '\0';
config->temp_dir[sizeof(config->temp_dir) - 1] = '\0';
config->config_file[sizeof(config->config_file) - 1] = '\0';

/* Initialize logging configuration - always use LOG_DAEMON as per @lumag's feedback */
config->log_config.facility = LOG_DAEMON;
config->log_config.min_level = LOG_INFO;
config->log_config.options = LOG_PID | LOG_CONS;
config->log_config.use_console = 0;
config->log_config.ident = "tqftpserv";
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But you already provided these default in the previous patch.

}

/**
* set_config_string() - Safely set a configuration string value
* @dest: Destination buffer
* @dest_size: Size of destination buffer
* @src: Source string
* @name: Configuration parameter name (for logging)
*/
static void set_config_string(char *dest, size_t dest_size, const char *src, const char *name)
{
if (strlen(src) >= dest_size) {
TQFTP_LOG_WARNING("Configuration value for '%s' too long, truncating", name);
}
strncpy(dest, src, dest_size - 1);
dest[dest_size - 1] = '\0';
}

/**
* tqftp_config_load_file() - Load configuration from file using libconfig
* @config: Configuration structure to populate
* @filename: Path to configuration file
*
* Uses libconfig library for robust configuration parsing with:
* - Proper syntax validation
* - Type checking
* - Hierarchical configuration support
* - Better error reporting
*
* Return: 0 on success, -1 on error
*/
int tqftp_config_load_file(struct tqftp_config *config, const char *filename)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yikes. If you want to have a config file, use one of existing libraries. I'd recommend libconfig. Or just use command line options.

Copy link
Copy Markdown
Contributor Author

@bhaskarv79 bhaskarv79 Aug 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried using libconfig and it is not available for all builds, does it make sense to copy libconfig in this project (I don't like this though)?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, it doesn't. What do you mean by it 'being not available for all builds'?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

'What do you mean by it 'being not available for all builds'?
I attempted to build on Android flavor and it failed, I did some experiments and understood that libconfig is present and it needed to be enabled from base config file. I have used libconfig and posted new change.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't really care about Android. But if you need libconfig there, just port it to external/ or platform/ or something like that.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having command line argument and a configuration file offers great flexibility, but what is the purpose/benefit?

Please include in the commit message "why" this patch is wanted.

{
config_t cfg;
const char *str_val;
int int_val;
int ret = 0;

config_init(&cfg);

/* Read the configuration file */
if (!config_read_file(&cfg, filename)) {
if (config_error_type(&cfg) != CONFIG_ERR_FILE_IO) {
TQFTP_LOG_ERR("Configuration error in %s:%d - %s",
filename, config_error_line(&cfg), config_error_text(&cfg));
} else {
/* File doesn't exist - this is not an error for optional config */
TQFTP_LOG_DEBUG("Configuration file %s not found, using defaults", filename);
}
config_destroy(&cfg);
return -1;
}

TQFTP_LOG_INFO("Loading configuration from %s", filename);

/* Load path configuration */
if (config_lookup_string(&cfg, "readonly_path", &str_val)) {
set_config_string(config->readonly_path, sizeof(config->readonly_path), str_val, "readonly_path");
TQFTP_LOG_DEBUG("Config: readonly_path = %s", str_val);
}

if (config_lookup_string(&cfg, "readwrite_path", &str_val)) {
set_config_string(config->readwrite_path, sizeof(config->readwrite_path), str_val, "readwrite_path");
TQFTP_LOG_DEBUG("Config: readwrite_path = %s", str_val);
}

if (config_lookup_string(&cfg, "firmware_base", &str_val)) {
set_config_string(config->firmware_base, sizeof(config->firmware_base), str_val, "firmware_base");
TQFTP_LOG_DEBUG("Config: firmware_base = %s", str_val);
}

if (config_lookup_string(&cfg, "updates_dir", &str_val)) {
set_config_string(config->updates_dir, sizeof(config->updates_dir), str_val, "updates_dir");
TQFTP_LOG_DEBUG("Config: updates_dir = %s", str_val);
}

if (config_lookup_string(&cfg, "temp_dir", &str_val)) {
set_config_string(config->temp_dir, sizeof(config->temp_dir), str_val, "temp_dir");
TQFTP_LOG_DEBUG("Config: temp_dir = %s", str_val);
}

/* Load logging configuration */
if (config_lookup_string(&cfg, "log_level", &str_val)) {
int level = tqftp_parse_log_level(str_val);
if (level >= 0) {
config->log_config.min_level = level;
TQFTP_LOG_DEBUG("Config: log_level = %s (%d)", str_val, level);
} else {
TQFTP_LOG_WARNING("Invalid log level '%s' in config file", str_val);
}
}

/* Log facility is always LOG_DAEMON as per @lumag's feedback */
if (config_lookup_string(&cfg, "log_facility", &str_val)) {
config->log_config.facility = LOG_DAEMON;
TQFTP_LOG_INFO("Log facility is fixed to 'daemon' (ignoring config value '%s')", str_val);
}

if (config_lookup_bool(&cfg, "log_console", &int_val)) {
config->log_config.use_console = int_val ? 1 : 0;
TQFTP_LOG_DEBUG("Config: log_console = %s", int_val ? "true" : "false");
}

/* Support both boolean and string formats for log_console */
if (config_lookup_string(&cfg, "log_console", &str_val)) {
if (strcasecmp(str_val, "yes") == 0 || strcasecmp(str_val, "true") == 0 || strcmp(str_val, "1") == 0) {
config->log_config.use_console = 1;
} else if (strcasecmp(str_val, "no") == 0 || strcasecmp(str_val, "false") == 0 || strcmp(str_val, "0") == 0) {
config->log_config.use_console = 0;
} else {
TQFTP_LOG_WARNING("Invalid log_console value '%s' (use true/false or yes/no)", str_val);
}
TQFTP_LOG_DEBUG("Config: log_console = %s", config->log_config.use_console ? "true" : "false");
}

config_destroy(&cfg);
TQFTP_LOG_INFO("Configuration loaded successfully from %s", filename);
return ret;
}

/**
* tqftp_config_print_help() - Print help message
* @progname: Program name
*/
void tqftp_config_print_help(const char *progname)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this in config.c ?

Also, add this incrementally - start by adding basic usage(), then add parts as you add features...don't just dump everything in at once.

{
printf("Usage: %s [OPTIONS]\n", progname);
printf("\nTQFTP Server - TFTP over QRTR\n");
printf("\nPath Configuration:\n");
printf(" --readonly-path PATH Set readonly path prefix (default: %s)\n", DEFAULT_READONLY_PATH);
printf(" --readwrite-path PATH Set readwrite path prefix (default: %s)\n", DEFAULT_READWRITE_PATH);
printf(" --firmware-base PATH Set firmware base directory (default: %s)\n", DEFAULT_FIRMWARE_BASE);
printf(" --updates-dir DIR Set updates directory name (default: %s)\n", DEFAULT_UPDATES_DIR);
printf(" --temp-dir PATH Set temporary directory (default: %s)\n", DEFAULT_TEMP_DIR);
printf("\nLogging Options:\n");
printf(" -l, --log-level LEVEL Set minimum log level\n");
printf(" (emerg, alert, crit, err, warn, notice, info, debug)\n");
printf(" Default: info\n");
printf(" -c, --console Log to console instead of syslog\n");
printf(" (Note: log facility is always 'daemon')\n");
printf("\nConfiguration File:\n");
printf(" --config FILE Load configuration from file (default: %s)\n", DEFAULT_CONFIG_FILE);
printf(" --no-config Don't load default configuration file\n");
printf("\nGeneral Options:\n");
printf(" -h, --help Show this help message\n");
printf("\nConfiguration File Format (libconfig syntax):\n");
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this included in the usage output?

printf(" # Comments start with # or //\n");
printf(" readonly_path = \"/readonly/firmware/image/\";\n");
printf(" readwrite_path = \"/readwrite/\";\n");
printf(" firmware_base = \"/lib/firmware/\";\n");
printf(" updates_dir = \"updates/\";\n");
printf(" temp_dir = \"/tmp/tqftpserv\";\n");
printf(" log_level = \"info\";\n");
printf(" log_console = false; # or true, yes, no\n");
printf("\nExamples:\n");
printf(" %s # Run with default settings\n", progname);
printf(" %s -l debug -c # Debug level logging to console\n", progname);
printf(" %s --firmware-base /custom/firmware # Use custom firmware directory\n", progname);
printf(" %s --config /etc/custom.conf # Use custom config file\n", progname);
printf("\n");
}

/**
* tqftp_config_parse_args() - Parse command line arguments
* @argc: Argument count
* @argv: Argument vector
* @config: Configuration structure to populate
*
* Return: 0 on success, 1 if help shown, -1 on error
*/
int tqftp_config_parse_args(int argc, char **argv, struct tqftp_config *config)
{
int opt;
int level;
int load_config_file = 1;

static struct option long_options[] = {
{"readonly-path", required_argument, 0, 1001},
{"readwrite-path", required_argument, 0, 1002},
{"firmware-base", required_argument, 0, 1003},
{"updates-dir", required_argument, 0, 1004},
{"temp-dir", required_argument, 0, 1005},
{"config", required_argument, 0, 1006},
{"no-config", no_argument, 0, 1007},
{"log-level", required_argument, 0, 'l'},
{"console", no_argument, 0, 'c'},
{"help", no_argument, 0, 'h'},
{0, 0, 0, 0}
};

while ((opt = getopt_long(argc, argv, "l:ch", long_options, NULL)) != -1) {
switch (opt) {
case 1001: /* --readonly-path */
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yuck. Nope. Use a normal way - a short single-letter param.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think long-only formats are fine, but the need for a comment gives away the obvious benefit of having enums for the values.

What I don't know is why we need (or why we want) these new options and why they are part of a commit with the title "add config file support".

set_config_string(config->readonly_path, sizeof(config->readonly_path), optarg, "readonly-path");
break;

case 1002: /* --readwrite-path */
set_config_string(config->readwrite_path, sizeof(config->readwrite_path), optarg, "readwrite-path");
break;

case 1003: /* --firmware-base */
set_config_string(config->firmware_base, sizeof(config->firmware_base), optarg, "firmware-base");
break;

case 1004: /* --updates-dir */
set_config_string(config->updates_dir, sizeof(config->updates_dir), optarg, "updates-dir");
break;

case 1005: /* --temp-dir */
set_config_string(config->temp_dir, sizeof(config->temp_dir), optarg, "temp-dir");
break;

case 1006: /* --config */
set_config_string(config->config_file, sizeof(config->config_file), optarg, "config");
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is config_file in the config object? It's pretty much a local variable in this function.

break;

case 1007: /* --no-config */
load_config_file = 0;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

--config foo.conf --no-config

break;

case 'l':
level = tqftp_parse_log_level(optarg);
if (level < 0) {
fprintf(stderr, "Invalid log level: %s\n", optarg);
return -1;
}
config->log_config.min_level = level;
break;

case 'c':
config->log_config.use_console = 1;
break;

case 'h':
tqftp_config_print_help(argv[0]);
return 1;

default:
tqftp_config_print_help(argv[0]);
return -1;
}
}

/* Load configuration file if requested */
if (load_config_file && access(config->config_file, R_OK) == 0) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So if I specify a config file that doesn't exist or I don't have permission to read, then I just silently will get the default settings?

tqftp_config_load_file(config, config->config_file);
}

return 0;
}
53 changes: 53 additions & 0 deletions config.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
// SPDX-License-Identifier: BSD-3-Clause
/*
* Copyright (c) Qualcomm Technologies, Inc. and/or its subsidiaries.
*/

#ifndef __CONFIG_H__
#define __CONFIG_H__

#include <limits.h>
#include "logging.h"

/* Configuration structure for TQFTP server */
struct tqftp_config {
/* Path configuration */
char readonly_path[PATH_MAX];
char readwrite_path[PATH_MAX];
char firmware_base[PATH_MAX];
char updates_dir[PATH_MAX];
char temp_dir[PATH_MAX];

/* Logging configuration */
struct tqftp_log_config log_config;

/* Configuration file path */
char config_file[PATH_MAX];
};

/* Default configuration values */
#define DEFAULT_READONLY_PATH "/readonly/firmware/image/"
#define DEFAULT_READWRITE_PATH "/readwrite/"

#ifndef ANDROID
#define DEFAULT_FIRMWARE_BASE "/lib/firmware/"
#define DEFAULT_TEMP_DIR "/tmp/tqftpserv"
#define DEFAULT_UPDATES_DIR "updates/"
#else
#define DEFAULT_FIRMWARE_BASE "/vendor/firmware/"
#define DEFAULT_TEMP_DIR "/data/vendor/tmp/tqftpserv"
#define DEFAULT_UPDATES_DIR "updates/"
#endif

#define DEFAULT_CONFIG_FILE "/etc/tqftpserv.conf"

/* Global configuration */
extern struct tqftp_config tqftp_config;

/* Function prototypes */
void tqftp_config_init_defaults(struct tqftp_config *config);
int tqftp_config_load_file(struct tqftp_config *config, const char *filename);
int tqftp_config_parse_args(int argc, char **argv, struct tqftp_config *config);
void tqftp_config_print_help(const char *progname);

#endif /* __CONFIG_H__ */
Loading