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

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

larsewi

Make sure to check return values of functions. It's OK to do best effort. I.e., we encounter an error but continue if it's safe. But in this case we need to cache the error so that we can later exit with failure. For some errors, like if we fail to create a directory, it does not make sense to try to fetch the files that should be in that directory afterwards.

larsewi · 2026-03-13T10:06:46Z

cf-net/cf-net.c

+static bool CFNetGetWithPerms(AgentConnection *conn, const char *remote_path,
+                          const char *local_path, bool print_stats)
+{
+    assert(conn != NULL && remote_path != NULL && local_path != NULL);


Try this:

Suggested change

assert(conn != NULL && remote_path != NULL && local_path != NULL);

assert(conn != NULL);

assert(remote_path != NULL);

assert(local_path != NULL);

Maybe it will silence the CodeQL warning. It's also OK to simply dismiss it. However, by fixing it, the next time someone touches this code, the warning does not pop back up. I'm fine with either way.

larsewi · 2026-03-13T10:50:48Z

cf-net/cf-net.c

+    bool* created = NULL;
+    bool force = false;
+    MakeParentDirectoryPerms(path, force, created, perms);


Personally I would just pass NULL here since you don't care if the directory was actually created or not. I get the idea that you want to improve readability with having "named" arguments. But in this case, it's more confusing than helpful.

Suggested change

bool* created = NULL;

bool force = false;

MakeParentDirectoryPerms(path, force, created, perms);

const bool force = false;

MakeParentDirectoryPerms(path, force, NULL, perms);

The intended use of this argument is:

const bool force = false; bool created; MakeParentDirectoryPerms(path, force, &created, perms); if (created) { // The directory was created } else { // The directory already existed }

Where, created is set to true by the function itself if the directory was created.

The MakeParentDirectoryPerms function also returns an error. Maybe you should check this error message, and not try to fetch the files that should be inside the directory if you failed to create it.

larsewi · 2026-03-13T10:51:43Z

cf-net/cf-net.c

+static int process_dir_recursive(AgentConnection *conn,
+                                  const char *remote_path,
+                                  const char *local_path,
+                                  bool has_output_path,
+                                  bool print_stats,
+                                  int limit)


Inconsistent indents

Suggested change

static int process_dir_recursive(AgentConnection *conn,

const char *remote_path,

const char *local_path,

bool has_output_path,

bool print_stats,

int limit)

static int process_dir_recursive(AgentConnection *conn,

const char *remote_path,

const char *local_path,

bool has_output_path,

bool print_stats,

int limit)

larsewi · 2026-03-13T10:55:40Z

cf-net/cf-net.c

+
+// Helper: Create local directory path
+static bool create_local_dir(const char *local_base, const char *subdir,
+                            bool has_output_path, mode_t perms)


It's not obvious what the has_output_path variable does. I think this function deserves a doxygen comment.

larsewi · 2026-03-13T10:57:55Z

cf-net/cf-net.c

+    }
+
+    int written;
+    Seq *items = ProtocolOpenDir(conn, remote_path);


Make sure to free this sequence in all the return paths

larsewi · 2026-03-13T11:12:34Z

cf-net/cf-net.c

+                                  bool print_stats,
+                                  int limit)
+{
+    if (0 > limit - 1)


This recursion limit test is confusing. Is not this the same as?

Suggested change

if (0 > limit - 1)

if (limit <= 0)

larsewi · 2026-03-13T11:13:47Z

cf-net/cf-net.c

+           case 'l':
+            {
+                char *lim;
+                long val = strtol(optarg, &lim, 10);


Need to check for overflow. We have functions that do this in libntech. I would suggest using those.

larsewi · 2026-03-13T11:15:47Z

cf-net/cf-net.c

+    args = &(args[optind]);
+    argc -= optind;
+    char *remote_dir = args[0];
+    char *base = basename(remote_dir);


basename may modify its argument

larsewi · 2026-03-13T11:19:15Z

cf-net/cf-net.c

+    assert(hostname != NULL);
+    assert(args != NULL);
+    char *local_dir = NULL;
+    int limit = 10;


Default should probably be more than 10.

Suggested change

int limit = 10;

int limit = 32;

larsewi · 2026-03-13T11:21:02Z

cf-net/cf-net.c

+        written = snprintf(path, sizeof(path), "%s/%s/%s/", cwd, local_base, subdir);
+    }
+
+    if (written < 0 || written >= sizeof(path))


Static checkers may complain that you are comparing singed and unsigned. Consider casting to unsigned (safe since the negative case is handled first)

Suggested change

if (written < 0 || written >= sizeof(path))

if (written < 0 || (size_t) written >= sizeof(path))

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

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

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

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

SimonThalvorsen force-pushed the CFE-2986 branch 2 times, most recently from d68100f to 047b068 Compare March 12, 2026 14:29

SimonThalvorsen requested a review from larsewi March 12, 2026 14:37

larsewi requested changes Mar 13, 2026

View reviewed changes

SimonThalvorsen force-pushed the CFE-2986 branch from 047b068 to 62eb2df Compare March 13, 2026 15:48

Added getdir command to cf-net, recursively copies directory

c2b59c7

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

SimonThalvorsen force-pushed the CFE-2986 branch from 62eb2df to c2b59c7 Compare March 13, 2026 15:49

	"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))
	{

-    assert(conn != NULL && remote_path != NULL && local_path != NULL);
+    assert(conn != NULL);
+    assert(remote_path != NULL);
+    assert(local_path != NULL);

	if (written < 0 \|\| written >= sizeof(path))
	if (written < 0 \|\| (size_t) written >= sizeof(path))

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!

Uh oh!

Uh oh!

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!

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!

Reviewers

olehermanse commented Mar 4, 2026 •

edited

Loading