Topic/syslog by bhaskarv79 · Pull Request #29 · linux-msm/tqftpserv

bhaskarv79 · 2025-08-10T10:09:26Z

Request to merge below changes:

Fix compilation issues when all warnings are enabled as errors
Syslog based logging for the server. This will allow debugging Server issues.
Server configuration through config file. This allows customizing the server for Linux build flavor
Sample configuration file. This provides reference to configuring the server.

lumag

All commit messages should start with the description of an issue: 'Foo ABC is bad, change it to be BAR.'

lumag · 2025-08-11T09:34:42Z

           tqftpserv_srcs,
-           dependencies : [qrtr_dep, zstd_dep],
+           dependencies : [qrtr, zstd_dep],
+           c_args : ['-g', '-Wall', '-Werror', '-Wsign-compare', '-Wunused-parameter', '-Wtype-limits', '-Wimplicit-function-declaration'],


This is obviosly an abuse of dependencies and it might break for non-gcc compilers. Please use add_global_arguments to add build options.

I added arguments using add_project_arguments

lumag · 2025-08-11T09:36:59Z

 	}

-	if (tsize && *tsize != -1) {
+	if (tsize && *tsize != (size_t)-1) {


Change tsize to ssize_t

lumag · 2025-08-11T09:37:08Z

 		/* Header (4 bytes) + data size */
 		send_len = 4 + response_size;
-		if (send_len > p - buf) {
+		if (send_len > (size_t)(p - buf)) {


changed send_len from size_t to ssize_t

lumag · 2025-08-11T09:37:38Z

 */

-/* For memfd_create */
-#define _GNU_SOURCE


Why? it was fine as is and it's not related to warnings

This macro needs to be defined for asprintf function usage in translate.c, so I moved it to meson.build

Please move it back and add another one in translate.c

lumag · 2025-08-11T09:38:46Z

 	}

-	if (write(output_file_fd, decompressed_buffer, decompressed_size) != decompressed_size) {
+	if (write(output_file_fd, decompressed_buffer, decompressed_size) != (ssize_t)decompressed_size) {


Change decompressed_size to size_t or ssize_t.

No, not done. I've asked to change its type to one of size_t types.

lumag · 2025-08-11T09:42:06Z

+ *
+ * Return: Log facility constant on success, -1 on error
+ */
+int tqftp_parse_log_facility(const char *facility_str)


Always use daemon, no need to let it be logged as "uucp".

lumag · 2025-08-11T09:53:52Z

+ * 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)


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

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.

Why? It's really easier to parse few command line options rather than having a config file parser.

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.

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.

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.

lumag · 2025-08-11T09:54:57Z

+ *
+ * Return: 0 on success, -1 on error
+ */
+int tqftp_config_load_file(struct tqftp_config *config, const char *filename)


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

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)?

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

'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.

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

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.

lumag · 2025-08-11T09:57:00Z

+
+/* Convenience macros for common logging patterns */
+#define TQFTP_LOG_ERROR(fmt, ...)   TQFTP_LOG_ERR(fmt, ##__VA_ARGS__)
+#define TQFTP_LOG_WARN(fmt, ...)    TQFTP_LOG_WARNING(fmt, ##__VA_ARGS__)


Why do we need these duplicates? Select one style and use it.

lumag · 2025-08-11T09:57:54Z

+#define TQFTP_LOG_PERROR(msg) TQFTP_LOG_ERR("%s: %s", msg, strerror(errno))
+
+/* Macro to log system errors with custom format */
+#define TQFTP_LOG_ERRNO(fmt, ...) TQFTP_LOG_ERR(fmt ": %s", ##__VA_ARGS__, strerror(errno))


Again, select one style instead of having two very similar ones.

…r issues Resolve multiple compilation issues triggered by -Wall and -Werror flags: - Multiple warnings in in zstd_decompress_file function in zstd-decompress.h. - Eliminate redefinition of zstd_decompress_file between zstd-decompress.h and zstd-decompress.c. - Address pointer type mismatch in tqftpserv.c by correcting char * vs uint8_t * assignment. - Resolve signedness comparison issues in tqftpserv.c for send_len and tsize checks. - Remove unused parameters argc and argv in main() function of tqftpserv.c. These changes ensure clean compilation and adherence to strict warning policies. Change-Id: I1539433ec5122e0e6fc3c8e0d02900a040bc21de Signed-off-by: Bhaskar Valaboju <quic_bhaskarv@quicinc.com>

Introduce a project-specific logging framework through syslog. Change-Id: Ie3399a58e9772f25979471cbf2f861ad1962d788 Signed-off-by: Bhaskar Valaboju <quic_bhaskarv@quicinc.com>

Integrate configuration management via config file and command-line arguments to customize server behavior. This allows runtime configuration of key filesystem paths including readonly_path, readwrite_path, firmware_base, updates_dir, and temp_dir. It also enables dynamic adjustment of the default log level, supporting flexible deployment across platforms like Linux and Android without requiring code changes. Change-Id: I788ce0eb1052f43872800bc0f2e0cc5d0a6b442b Signed-off-by: Bhaskar Valaboju <quic_bhaskarv@quicinc.com>

Integrate syslog-based logging into tqftpserv source files. Logging now leverages system-level facilities for consistent output and easier debugging. Signed-off-by: Bhaskar Valaboju <quic_bhaskarv@quicinc.com>

Integrate configuration management via config file and command-line arguments to customize server behavior. This allows runtime configuration of key filesystem paths including readonly_path, readwrite_path, firmware_base, updates_dir, and temp_dir. It also enables dynamic adjustment of the default log level, supporting flexible deployment across platforms like Linux and Android without requiring code changes. Signed-off-by: Bhaskar Valaboju <quic_bhaskarv@quicinc.com>

Provide a sample configuration file to support runtime customization of server paths and log level settings. Signed-off-by: Bhaskar Valaboju <quic_bhaskarv@quicinc.com>

bhaskarv79 · 2025-08-26T12:02:24Z

I addressed all the comments, it took time to validate on all flavors.

Integrate configuration management via config file and command-line arguments to customize server behavior. This allows runtime configuration of key filesystem paths including readonly_path, readwrite_path, firmware_base, updates_dir, and temp_dir. It also enables dynamic adjustment of the default log level, supporting flexible deployment across platforms like Linux and Android without requiring code changes. Change-Id: I788ce0eb1052f43872800bc0f2e0cc5d0a6b442b Signed-off-by: Bhaskar Valaboju <quic_bhaskarv@quicinc.com>

bhaskarv79 · 2025-09-03T03:00:44Z

Gentle Remainder!!!

bhaskarv79 · 2025-09-22T09:27:55Z

Gentle Remainder !!!

bhaskarv79 · 2025-09-22T09:29:59Z

Gentle Remainder, @lumag

lumag · 2025-12-24T04:29:44Z

 prefix = get_option('prefix')

+# Add specific warning flags not covered by warning_level=3
+add_project_arguments('-Wsign-compare', '-Wunused-parameter', '-Wtype-limits', '-Wimplicit-function-declaration', language : 'c')


Why? Why did you select these flags?

lumag · 2025-12-24T04:30:00Z

@@ -4,12 +4,16 @@ project('tqftpserv',
        'c',


Please drop Gerrit tags. We don't use Gerrit here.

lumag · 2025-12-24T04:31:33Z

 	}

-	if (write(output_file_fd, decompressed_buffer, decompressed_size) != decompressed_size) {
+	if (write(output_file_fd, decompressed_buffer, decompressed_size) != (ssize_t)decompressed_size) {


No, not done. I've asked to change its type to one of size_t types.

lumag · 2025-12-24T04:35:29Z

+ * 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)


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.

lumag · 2025-12-24T04:36:42Z

+ *
+ * Return: 0 on success, -1 on error
+ */
+int tqftp_config_load_file(struct tqftp_config *config, const char *filename)


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

lumag · 2025-12-24T04:37:24Z

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


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

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".

quic-bjorande

There are some concrete issues that needs to be addressed in tqftpserv, adding 706 lines of configuration flexibility does not seem to be the most pressing issue...

quic-bjorande · 2026-01-13T21:23:04Z

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


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".

quic-bjorande · 2026-01-13T21:26:56Z

+ *
+ * Return: 0 on success, -1 on error
+ */
+int tqftp_config_load_file(struct tqftp_config *config, const char *filename)


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.

quic-bjorande · 2026-01-13T21:32:21Z

@@ -4,12 +4,16 @@ project('tqftpserv',
        'c',


This patch seems to have various interesting and useful changes lumped together in one patch, and as it's part of the same PR as everything else none of it can be merged without agreeing to it all...

Split things into multiple patches and if possible multiple PRs.

quic-bjorande · 2026-01-13T21:36:59Z

@@ -0,0 +1,122 @@
+// SPDX-License-Identifier: BSD-3-Clause-Clear


The commit message fails to establish the problem this resolves.

quic-bjorande · 2026-01-13T21:37:50Z

+	{"warning", LOG_WARNING},
+	{"notice", LOG_NOTICE},
+	{"info", LOG_INFO},
+	{"debug", LOG_DEBUG},


12 different log levels!? What?!

quic-bjorande · 2026-01-13T21:55:03Z

+	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";


But you already provided these default in the previous patch.

quic-bjorande · 2026-01-13T21:57:00Z

 	len = pread(client->fd, p, client->blksize, offset);
 	if (len < 0) {
-		printf("[TQFTP] failed to read data\n");
+		TQFTP_LOG_ERR("failed to read data");


Please just call the function directly, so we can have this lowercase.

quic-bjorande · 2026-01-13T21:58:51Z

+	if (ret == 1) {
+		/* Help was shown */
+		return 0;
+	} else if (ret < 0) {


Both conditional arms here are generally handled by one entity, which just calls exit().

quic-bjorande · 2026-01-13T22:00:09Z

@@ -16,6 +16,7 @@
 #include "list.h"


Avoid adding a number of patches that builds up a bunch of functionality and then just throw it all in the mix at the end. If this change causes an issue, the bug is in any of the patches leading up to this change.

Also avoid doing multiple things in the patch itself. The commit message quite clearly tells that this adds config file support. (It also adds command line arguments, setting log level and other things, but you need to read the full patch to learn that)

quic-bjorande · 2026-01-13T22:06:02Z

        "translate.c",
    ],
-    shared_libs: ["libqrtr"],
+    shared_libs: ["libqrtr", "libconfig"],


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.

lumag requested changes Aug 11, 2025

View reviewed changes

bhaskarv79 added 6 commits August 26, 2025 13:49

tqftpserv: Add Syslog-based logging system

a6b58e3

Introduce a project-specific logging framework through syslog. Change-Id: Ie3399a58e9772f25979471cbf2f861ad1962d788 Signed-off-by: Bhaskar Valaboju <quic_bhaskarv@quicinc.com>

tqftpserv: Enable syslog logging in tqftpserv

eb773fa

Integrate syslog-based logging into tqftpserv source files. Logging now leverages system-level facilities for consistent output and easier debugging. Signed-off-by: Bhaskar Valaboju <quic_bhaskarv@quicinc.com>

tqftpserv: Add sample configuration file for tqftpserv

d2e90ce

Provide a sample configuration file to support runtime customization of server paths and log level settings. Signed-off-by: Bhaskar Valaboju <quic_bhaskarv@quicinc.com>

bhaskarv79 force-pushed the topic/syslog branch from 4551b96 to d2e90ce Compare August 26, 2025 08:43

lumag requested changes Dec 24, 2025

View reviewed changes

quic-bjorande suggested changes Jan 13, 2026

View reviewed changes

Pradeep-pvk mentioned this pull request Jan 28, 2026

recipes-support: Add new recipe for tftp-server qualcomm-linux/meta-qcom#1445

Open

andersson mentioned this pull request Feb 2, 2026

readwrite requests are mapped/written to non-persistent path #33

Open

		@@ -0,0 +1,122 @@
		// SPDX-License-Identifier: BSD-3-Clause-Clear

Conversation

bhaskarv79 commented Aug 10, 2025

Uh oh!

lumag left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bhaskarv79 Aug 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bhaskarv79 commented Aug 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bhaskarv79 commented Sep 3, 2025

Uh oh!

bhaskarv79 commented Sep 22, 2025

Uh oh!

bhaskarv79 commented Sep 22, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lumag left a comment •

edited

Loading

bhaskarv79 Aug 13, 2025 •

edited

Loading

bhaskarv79 commented Aug 26, 2025 •

edited

Loading