Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
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
1 change: 1 addition & 0 deletions Android.bp
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ cc_binary {
name: "tqftpserv",
vendor: true,
srcs: [
"logging.c",
"tqftpserv.c",
"translate.c",
],
Expand Down
122 changes: 122 additions & 0 deletions logging.c
Original file line number Diff line number Diff line change
@@ -0,0 +1,122 @@
// SPDX-License-Identifier: BSD-3-Clause-Clear
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.

The commit message fails to establish the problem this resolves.

/*
* Copyright (c) Qualcomm Technologies, Inc. and/or its subsidiaries.
*/

#include <stdarg.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <time.h>
#include "logging.h"

/* Global logging configuration */
struct tqftp_log_config tqftp_log_cfg = {
.facility = LOG_DAEMON,
.min_level = LOG_INFO,
.options = LOG_PID | LOG_CONS,
.use_console = 0,
.ident = "tqftpserv"
};

/* Log level name mappings */
static const struct {
const char *name;
int level;
} log_levels[] = {
{"emerg", LOG_EMERG},
{"emergency", LOG_EMERG},
{"alert", LOG_ALERT},
{"crit", LOG_CRIT},
{"critical", LOG_CRIT},
{"err", LOG_ERR},
{"error", LOG_ERR},
{"warn", LOG_WARNING},
{"warning", LOG_WARNING},
{"notice", LOG_NOTICE},
{"info", LOG_INFO},
{"debug", LOG_DEBUG},
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.

12 different log levels!? What?!

{NULL, -1}
};


/**
* tqftp_log_init() - Initialize logging with default configuration
* @ident: Program identifier for log messages
*/
void tqftp_log_init(const char *ident)
{
tqftp_log_cfg.ident = ident;
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.

In what circumstances does this need/benefit from being anything other than "tqftpserv"?

options is LOG_PID | LOG_CONS
facility is LOG_DAEMON

No need to complicate it further until those are actually variable.

openlog(tqftp_log_cfg.ident, tqftp_log_cfg.options, tqftp_log_cfg.facility);
}

/**
* tqftp_log_init_with_config() - Initialize logging with custom configuration
* @config: Logging configuration structure
*/
void tqftp_log_init_with_config(struct tqftp_log_config *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.

I'd suggest using function calls instead of passing config around. I.e. set_min_level, use_console, etc.

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.

configuration file will allow to run the server on various linux flavors without changing the server code. I added configuration support for command line arguments as well, I will remove them and have server access paths only.

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? It's really easier to parse few command line options rather than having a config file parser.

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.

We can add command line option, if we want to change the paths (based on the build type, android, LE, Ubuntu, etc) then we have to modify service file (or equivalent) that starts the server and this service file needs to be created based on variant.
With configuration file as well it would be similar (multiple configuration files) however, config files can be modified in on the fly for debugging. There are other tuning parameters that needs to be introduced, so I thought configuration file would be better than adding too many command line arguments.
Lets discuss which is better.

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've looked again at this. I think the config is an overkill. Likewise the multi-level logging system is a definite overkill for something as simple as tqftpserv. Our systems use journald, so everything gets logged to the journal. I don't see why you can't use that to capture all the logging messages.

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 agree, these patches just introduce flexibility for the sake of having flexibility - and all flexibility at once.

Please add flexibility where you need it, not everywhere for the sake of it.

{
if (config) {
tqftp_log_cfg = *config;
}

if (!tqftp_log_cfg.use_console) {
openlog(tqftp_log_cfg.ident, tqftp_log_cfg.options, tqftp_log_cfg.facility);
}
}

/**
* tqftp_log() - Internal logging function
* @priority: Syslog priority level
* @fmt: Format string
* @...: Format arguments
*/
void tqftp_log(int priority, const char *fmt, ...)
{
va_list args;
char buffer[1024];
time_t now;
struct tm *tm_info;
char time_str[64];

va_start(args, fmt);

if (tqftp_log_cfg.use_console) {
/* Log to console with timestamp */
time(&now);
tm_info = localtime(&now);
strftime(time_str, sizeof(time_str), "%Y-%m-%d %H:%M:%S", tm_info);

vsnprintf(buffer, sizeof(buffer), fmt, args);
fprintf(stderr, "%s [TQFTP] %s\n", time_str, buffer);
fflush(stderr);
} else {
/* Log to syslog */
vsnprintf(buffer, sizeof(buffer), fmt, args);
syslog(priority, "[TQFTP] %s", buffer);
}

va_end(args);
}

/**
* tqftp_parse_log_level() - Parse log level string
* @level_str: Log level string (e.g., "info", "debug", "error")
*
* Return: Log level constant on success, -1 on error
*/
int tqftp_parse_log_level(const char *level_str)
{
int i;

if (!level_str)
return -1;

for (i = 0; log_levels[i].name; i++) {
if (!strcasecmp(level_str, log_levels[i].name)) {
return log_levels[i].level;
}
}

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

#ifndef __LOGGING_H__
#define __LOGGING_H__

#include <syslog.h>
#include <stdio.h>
#include <errno.h>
#include <string.h>

/* Logging configuration structure */
struct tqftp_log_config {
int facility;
int min_level;
int options;
int use_console;
const char *ident;
};

/* Global logging configuration */
extern struct tqftp_log_config tqftp_log_cfg;

/* Initialize syslog for the application */
#define LOG_INIT(ident) tqftp_log_init(ident)
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.

These macros are just creating aliases for each function, making the code harder to read. Keep it simple.

#define LOG_INIT_WITH_CONFIG(config) tqftp_log_init_with_config(config)

/* Close syslog */
#define LOG_CLOSE() closelog()

/* Function prototypes */
void tqftp_log_init(const char *ident);
void tqftp_log_init_with_config(struct tqftp_log_config *config);
int tqftp_parse_log_level(const char *level_str);

/* Internal logging function */
void tqftp_log(int priority, const char *fmt, ...);

/* Project-specific logging macros with different priority levels */
#define TQFTP_LOG_EMERG(fmt, ...) do { if (LOG_EMERG >= tqftp_log_cfg.min_level) tqftp_log(LOG_EMERG, fmt, ##__VA_ARGS__); } while(0)
#define TQFTP_LOG_ALERT(fmt, ...) do { if (LOG_ALERT >= tqftp_log_cfg.min_level) tqftp_log(LOG_ALERT, fmt, ##__VA_ARGS__); } while(0)
#define TQFTP_LOG_CRIT(fmt, ...) do { if (LOG_CRIT >= tqftp_log_cfg.min_level) tqftp_log(LOG_CRIT, fmt, ##__VA_ARGS__); } while(0)
#define TQFTP_LOG_ERR(fmt, ...) do { if (LOG_ERR >= tqftp_log_cfg.min_level) tqftp_log(LOG_ERR, fmt, ##__VA_ARGS__); } while(0)
#define TQFTP_LOG_WARNING(fmt, ...) do { if (LOG_WARNING >= tqftp_log_cfg.min_level) tqftp_log(LOG_WARNING, fmt, ##__VA_ARGS__); } while(0)
#define TQFTP_LOG_NOTICE(fmt, ...) do { if (LOG_NOTICE >= tqftp_log_cfg.min_level) tqftp_log(LOG_NOTICE, fmt, ##__VA_ARGS__); } while(0)
#define TQFTP_LOG_INFO(fmt, ...) do { if (LOG_INFO >= tqftp_log_cfg.min_level) tqftp_log(LOG_INFO, fmt, ##__VA_ARGS__); } while(0)
#define TQFTP_LOG_DEBUG(fmt, ...) do { if (LOG_DEBUG >= tqftp_log_cfg.min_level) tqftp_log(LOG_DEBUG, fmt, ##__VA_ARGS__); } while(0)

#endif /* __LOGGING_H__ */
3 changes: 2 additions & 1 deletion meson.build
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,8 @@ endif

qrtr_dep = dependency('qrtr')

tqftpserv_srcs = ['translate.c',
tqftpserv_srcs = ['logging.c',
'translate.c',
'tqftpserv.c',
'zstd-decompress.c']
executable('tqftpserv',
Expand Down