CFE-2986: Added getdir command to cf-net, copies directory and files by SimonThalvorsen · Pull Request #6049 · cfengine/core

SimonThalvorsen · 2026-03-02T10:02:31Z

Ticket: CFE-2986
Changelog: Title

cf-net/cf-net.c

larsewi

It would be nicer if we extended cf-net get to get both files and directories, instead of having two separate sub-commands. Please investigate how rsync does this and try to do it in a similar way here.

olehermanse · 2026-03-04T00:02:37Z

It would be nicer if we extended cf-net get to get both files and directories, instead of having two separate sub-commands. Please investigate how rsync does this and try to do it in a similar way here.

@larsewi on the other hand, cf-net was written mainly as a protocol testing tool, and the commands mirror their protocol command names. If the GET protocol command in CFEngine network protocol can only GET a file, it might be a bit confusing that cf-net get is something very different.

There is also some advantage to not being too dynamic with this stuff. If you make a CLI that forces the user to specify if they are copying a file or a folder, you can prevent some annoying / confusing situations.

We could for example make a new subcommand, copy;

$ cf-net copy --file promises.cf
Copying file 1.2.3.4:/var/cfengine/masterfiles/promises.cf -> ./promises.cf

$ cf-net copy --file services/
Error: 'services/' does not look like a file, did you mean --directory?

$ cf-net copy --file services/.
Error: 'services/.' does not look like a file, did you mean --directory?

$ cf-net copy --directory services/
Copying directory 1.2.3.4:/var/cfengine/masterfiles/services/ -> ./services/

$ cf-net copy services/
Copying directory 1.2.3.4:/var/cfengine/masterfiles/services/ -> ./services/

$ cf-net copy services
Warning: Ambiguous if 'services' is a file or a directory - consider adding --file or --directory.
Copying directory 1.2.3.4:/var/cfengine/masterfiles/services/ -> ./services/

larsewi

Please break this up a little. Maybe a recursive solution would be more elegant given that it is a recursive problem.

larsewi · 2026-03-05T12:08:40Z

cf-net/cf-net.c

+        filedata->print_stats = opts->print_stats;
+
+        filedata->ret = ProtocolStatGet(conn, filedata->remote_file,
+                filedata->local_file, 0644, filedata->print_stats);


Maybe you should use the remote file permissions (available from the ProtocolStat) instead?

larsewi · 2026-03-05T12:09:23Z

cf-net/cf-net.c

+        char curr_dir[PATH_MAX];
+        snprintf(curr_dir, sizeof(curr_dir), "%s/%s", local_dir, curr);
+
+        GetFileData *filedata = calloc(1, sizeof(GetFileData));


This does not need to be heap allocated

larsewi · 2026-03-05T12:10:24Z

cf-net/cf-net.c

+            continue;
+        }
+
+        snprintf(filedata->local_file, PATH_MAX, "%s", curr_dir);


Please check for truncation on all the calls to snprintf

larsewi · 2026-03-05T12:12:29Z

cf-net/cf-net.c

+                return invalid_command("getdir");
+                break;


Unreachable break after return

larsewi · 2026-03-05T12:13:00Z

cf-net/cf-net.c

+                if (local_dir != NULL)
+                {
+                    Log(LOG_LEVEL_INFO,
+                        "Warning: multiple occurences of -o in command, "\


Suggested change

"Warning: multiple occurences of -o in command, "\

"Warning: multiple occurrences of -o in command, "\

cf-net/cf-net.c

+        Log(LOG_LEVEL_ERR, "Failed to get remote file: %s:%s",
+            conn->this_server, remote_path);
+        return false;
+    }


cf-net/cf-net.c

+}
+
+// Helper: Create local directory path
+static bool create_local_dir(const char *local_base, const char *subdir,


cf-net/cf-net.c

+    char *local_dir = NULL;
+
+    int argc = 0;
+    while (args[argc] != NULL)


larsewi

There should be a recursion depth limit to prevent a potential malicious or misconfigured server with symlink loops (or deeply nested dirs) to cause stack overflow.

We should probably stick to a maximum path size for the same reasons above.

larsewi · 2026-03-10T11:47:45Z

cf-net/cf-net.c

+        char local_full[PATH_MAX];
+        written = snprintf(local_full, sizeof(local_full), "%s/%s", local_path, item);
+        if (written < 0 || written >= sizeof(local_full)) {
+            Log(LOG_LEVEL_ERR, "Path too long for full local path: %s",local_full);


Here you print the truncated path and not the intended full path. This could be a bit misleading.

larsewi · 2026-03-10T11:49:09Z

cf-net/cf-net.c

+        if (written < 0 || written >= len) {
+            Log(LOG_LEVEL_ERR, "Path too long for local path: %s", temp);
+            free(local_dir);
+            return -1;


temp is leaked here

larsewi · 2026-03-10T11:50:31Z

cf-net/cf-net.c

+            free(local_dir);
+            return -1;
+        }
+        int written = snprintf(temp, len, "%s/%s", local_dir, basename(remote_dir));


Consider using StringFormat()

larsewi · 2026-03-10T11:51:08Z

cf-net/cf-net.c

+    char *remote_dir = args[0];
+    if (specified_path)
+    {
+        size_t len = strlen(local_dir) + strlen(basename(remote_dir)) + 2; // / and '\0'


basename() may modify it's argument

larsewi · 2026-03-10T11:53:49Z

cf-net/cf-net.c

+}
+
+// Helper: Recursively process directory entries
+static void process_dir_recursive(AgentConnection *conn,


The function returns void. If all files fail to download, CFNetGetDir still returns 0 (success).

I do not see why this is an issue, failing to stat/get/mkdir will all log their respective errors. So you would rather it return -1 on any one error instead of continue?

If you were to use it in a script, then it would be easier to check the exit value rather then parsing log messages. Does the other commands exit with 0 on failure?

larsewi · 2026-03-10T11:54:57Z

cf-net/cf-net.c

 }

+// Helper: Get a single file with permissions
+static bool CFNetGetWithPerms(AgentConnection *conn, const char *remote_path,


File permissions are preserved via ProtocolGet, but created directories don't get the remote directory's permissions applied.

larsewi · 2026-03-10T11:55:16Z

cf-net/cf-net.c

+    SeqDestroy(items);
+}
+
+static int CFNetGetDir( CFNetOptions *opts, const char *hostname, char **args)


Suggested change

static int CFNetGetDir( CFNetOptions *opts, const char *hostname, char **args)

static int CFNetGetDir(CFNetOptions *opts, const char *hostname, char **args)

larsewi · 2026-03-10T11:58:02Z

cf-net/cf-net.c

+            return -1;
+        }
+        int written = snprintf(temp, len, "%s/%s", local_dir, basename(remote_dir));
+        if (written < 0 || written >= len) {


Suggested change

if (written < 0 || written >= len) {

if (written < 0 || written >= len)

{

larsewi · 2026-03-10T11:58:23Z

cf-net/cf-net.c

+    assert(conn != NULL && remote_path != NULL && local_path != NULL);
+
+    struct stat perms;
+    if (!ProtocolStat(conn, remote_path, &perms)) {


Try to be consistent with the braces. CONTRIBUTING.md says to put them on a new line. Look through the rest of the code, there are more cases like this.

Suggested change

if (!ProtocolStat(conn, remote_path, &perms)) {

if (!ProtocolStat(conn, remote_path, &perms))

{

larsewi · 2026-03-10T12:01:22Z

cf-net/cf-net.c

+
+    if (has_output_path) {
+        written = snprintf(path, sizeof(path), "%s/%s/", local_base, subdir);
+    } else {


Use Allman style, not K&R style (} else {)

Ticket: CFE-2986 Changelog: Title Signed-off-by: Simon Halvorsen <simon.halvorsen@northern.tech>

SimonThalvorsen requested a review from larsewi March 2, 2026 10:02

github-advanced-security bot found potential problems Mar 2, 2026

View reviewed changes

cf-net/cf-net.c Fixed Show fixed Hide fixed

cf-net/cf-net.c Fixed Show fixed Hide fixed

SimonThalvorsen force-pushed the CFE-2986 branch from f7cebad to 40a6c09 Compare March 2, 2026 10:40

larsewi changed the title ~~Added getdir command to cf-net, copies directory and files~~ CFE-2986: Added getdir command to cf-net, copies directory and files Mar 2, 2026

larsewi requested changes Mar 2, 2026

View reviewed changes

SimonThalvorsen force-pushed the CFE-2986 branch from 40a6c09 to 2bd2729 Compare March 4, 2026 14:58

larsewi requested changes Mar 5, 2026

View reviewed changes

SimonThalvorsen force-pushed the CFE-2986 branch from 2bd2729 to e9f4cb3 Compare March 6, 2026 13:53

github-advanced-security bot found potential problems Mar 6, 2026

View reviewed changes

SimonThalvorsen force-pushed the CFE-2986 branch from e9f4cb3 to 5b55d16 Compare March 6, 2026 14:12

github-advanced-security bot found potential problems Mar 6, 2026

View reviewed changes

cf-net/cf-net.c

char *local_dir = NULL;

int argc = 0;

while (args[argc] != NULL)

Check warning

Code scanning / CodeQL

Poorly documented large function Warning

Poorly documented function: fewer than 2% comments for a function of 115 lines.

SimonThalvorsen requested a review from larsewi March 9, 2026 14:42

larsewi requested changes Mar 10, 2026

View reviewed changes

Added getdir command to cf-net, recursively copies directory

d68100f

Ticket: CFE-2986 Changelog: Title Signed-off-by: Simon Halvorsen <simon.halvorsen@northern.tech>

SimonThalvorsen force-pushed the CFE-2986 branch from 5b55d16 to d68100f Compare March 11, 2026 13:40

	"Warning: multiple occurences of -o in command, "\
	"Warning: multiple occurrences of -o in command, "\

	static int CFNetGetDir( CFNetOptions opts, const char hostname, char **args)
	static int CFNetGetDir(CFNetOptions opts, const char hostname, char **args)

	if (written < 0 \|\| written >= len) {
	if (written < 0 \|\| written >= len)
	{

	if (!ProtocolStat(conn, remote_path, &perms)) {
	if (!ProtocolStat(conn, remote_path, &perms))
	{

Conversation

SimonThalvorsen commented Mar 2, 2026

Uh oh!

Uh oh!

Uh oh!

larsewi left a comment

Choose a reason for hiding this comment

Uh oh!

olehermanse commented Mar 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

larsewi left a comment

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!

Check notice

Check notice

Uh oh!

Uh oh!

Check warning

larsewi left a comment

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!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

olehermanse commented Mar 4, 2026 •

edited

Loading