Some enhancements to the zosfile unit

[ChongZhou-Broadcom]

Purpose of the PR

This is an extension (a super set) of the old unmerged PR #633 . The old one was closed.

The previous PR aligned the directoryDeleteRecursive() function with the system command rm -rf with regards to the handling of symbolic links.

After that, I noticed that the directoryCopy() function has the same issue about symbolic links.

The issue can be summarized as follows: when deleting or copying a directory, symbolic links within the source directory should be unlinked or recreated in the target directory, rather than deleting or copying the files or directories to which those links point.

This PR fixes the inconsistant behavior of the previous implementation.

Additionally, it also extracts the directory traversal code into a function shared by all directory operation functions.

Summary of major changes

• directoryDeleteRecursive() and directoryCopy() no longer follow links, now they only delete or create links;
• New function directoryListEntries() was added and shared by all directory functions.
• New function fileReadLink2() was added to implement a convenient and consistent way for checking if a path is a symlink and what the real path is.

Hints for unit tests

1. The following functions should be tested: directoryDeleteRecursive, directoryCopy, directoryChangeModeRecursive, directoryChangeOwnerRecursive, fileReadLink2, directoryListEntries;
2. Functions listed in point 1 can be tested in a group in a stable order;
3. The test requires two valid system users and groups. (user1 from group1 and user2 from group2)
4. Before all tests, the test framework should create the following directory structure as the test data:

test_data/
test_data/dir1/
test_data/dir1/file1
test_data/dir2/
test_data/dir2/file2
test_data/dir2/file1 --(symlink)--> test_data/dir1/file1
test_data/dir2/dir2.1/
test_data/dir2/dir2.1/file3
test_data/dir2/dir2.1/dir1 --(symlink)--> test_data/dir1/

All files must be in mode 644, all directories must be in mode 755.
All files and directories must owned by user1 and group1.

5. test 1: directoryListEntries_should_return_expected_entry_list

int rc, rsn;
UnixDirectoryEntries entries;
directoryListEntries("test_data/dir2", &entries, &rc, &rsn);
EXPECT_EQUAL(rc, 0);
EXPECT_EQUAL(rsn, 0);
EXPECT_EQUAL(entries.numEntries, 3);
EXPECT_ARRAY_CONTAINS(entries.entryArray, entries.numEntries, "file1");
EXPECT_ARRAY_CONTAINS(entries.entryArray, entries.numEntries, "file2");
EXPECT_ARRAY_CONTAINS(entries.entryArray, entries.numEntries, "dir3");

6. test2: fileReadLink2_should_return_expected_type

int rc, rsn;
ReadLinkResult result;
fileReadLink2("test_data", &result, &rc, &rsn);
EXPECT_EQUAL(rc, 0);
EXPECT_EQUAL(rsn, 0);
EXPECT_EQUAL(result.type, RL2_TypeRealDir);
fileReadLink2("test_data/dir1/file1", &result, &rc, &rsn);
EXPECT_EQUAL(rc, 0);
EXPECT_EQUAL(rsn, 0);
EXPECT_EQUAL(result.type, RL2_TypeRealFile);
fileReadLink2("test_data/dir2/file1", &result, &rc, &rsn);
EXPECT_EQUAL(rc, 0);
EXPECT_EQUAL(rsn, 0);
EXPECT_EQUAL(result.type, RL2_TypeSymlink);
EXPECT_EQUAL_STR(result.realPath, "test_data/dir1/file1");
fileReadLink2("test_data/dir2/dir2.1/dir1", &result, &rc, &rsn);
EXPECT_EQUAL(rc, 0);
EXPECT_EQUAL(rsn, 0);
EXPECT_EQUAL(result.type, RL2_TypeSymlink);
EXPECT_EQUAL_STR(result.realPath, "test_data/dir1/");

7. test3: directoryCopy_should_create_identical_dir3_from_dir2

int rc, rsn;
directoryCopy("test_data/dir2", "test_data/dir3", &rc, &rsn);
EXPECT_EQUAL(rc, 0);
EXPECT_EQUAL(rsn, 0);
// must also check the shape of test_data/dir2 and test_data/dir3, they must be completely idential

8. test4: directoryChangeModeRecursive_should_set_mode

int rc, rsn;
int mode777 = S_IRWXU2 | S_IRWXG | S_IRWXO;
directoryChangeModeRecursive("test_data/dir3/dir2.1", CHANGE_MODE_RECURSIVE, mode777, &rc, &rsn);
EXPECT_EQUAL(rc, 0);
EXPECT_EQUAL(rsn, 0);
FileInfo info;
fileInfo("test_data/dir3/dir2.1/file3", &info, &rc, &rsn);
EXPECT_EQUAL(rc, 0);
EXPECT_EQUAL(rsn, 0);
EXPECT_EQUAL(fileUnixMode(info), mode);
fileInfo("test_data/dir1", &info, &rc, &rsn);
EXPECT_EQUAL(rc, 0);
EXPECT_EQUAL(rsn, 0);
EXPECT_EQUAL(fileUnixMode(info), mode);
fileInfo("test_data/dir1/file1", &info, &rc, &rsn);
EXPECT_EQUAL(rc, 0);
EXPECT_EQUAL(rsn, 0);
EXPECT_EQUAL(fileUnixMode(info), mode);

9. test5: directoryChangeModeRecursive_should_change_owner

int user2, group2;
int rc, rsn;
int directoryChangeOwnerRecursive(NULL, 0, "test_data/dir3/dir2.1", user2, group2, TRUE, NULL, &rc, &rsn);
EXPECT_EQUAL(rc, 0);
EXPECT_EQUAL(rsn, 0);
FileInfo info;
fileInfo("test_data/dir3/dir2.1/", &info, &rc, &rsn);
EXPECT_EQUAL(rc, 0);
EXPECT_EQUAL(rsn, 0);
EXPECT_EQUAL(info.ownerUID, user2);
EXPECT_EQUAL(info.ownerGID, group2);
fileInfo("test_data/dir3/dir2.1/file3", &info, &rc, &rsn);
EXPECT_EQUAL(rc, 0);
EXPECT_EQUAL(rsn, 0);
EXPECT_EQUAL(info.ownerUID, user2);
EXPECT_EQUAL(info.ownerGID, group2);
fileInfo("test_data/dir1/", &info, &rc, &rsn);
EXPECT_EQUAL(rc, 0);
EXPECT_EQUAL(rsn, 0);
EXPECT_EQUAL(info.ownerUID, user2);
EXPECT_EQUAL(info.ownerGID, group2);
fileInfo("test_data/dir1/file1", &info, &rc, &rsn);
EXPECT_EQUAL(rc, 0);
EXPECT_EQUAL(rsn, 0);
EXPECT_EQUAL(info.ownerUID, user2);
EXPECT_EQUAL(info.ownerGID, group2);

10. test6: directoryDelete_should_delete_dir3

int rc, rsn;
directoryDelete("test_data/dir3", &rc, &rsn);
EXPECT_EQUAL(rc, 0);
EXPECT_EQUAL(rsn, 0);
FileInfo info;
fileInfo("test_data/dir3", &info, &rc, &rsn);
EXPECT_EQUAL(rc, ENOENT);
FileInfo info;
fileInfo("test_data/dir1", &info, &rc, &rsn);
EXPECT_EQUAL(rc, 0);
EXPECT_EQUAL(rsn, 0);
fileInfo("test_data/dir1/file1", &info, &rc, &rsn);
EXPECT_EQUAL(rc, 0);
EXPECT_EQUAL(rsn, 0);

[JoeNemo]

Can you turn the "hints for testing" into tests that are part of this PR. The scripts should go in a subdir of zowe-common-c/tests that you create and one script should build the directory structure that can reproduce the problem, another can compile a tiny program to run the test. That would allow me show that the bug existed and was cleared by this fix by running the test in a local repo that does NOT have this fix in it yet.

[ChongZhou-Broadcom]

After seeing your proposal to convert "hints for testing" into actual test cases, I’ve been wondering: since you are already building a ZSS-based regression testing framework, do we still need unit tests?

Assuming for a moment that we do need unit tests, could they also be structured as a framework? Here is the concept:

• It would be an executable program with a single main entry point (essentially, a program similar to ZSS).
• It would not be a ZSS server, so it might not require complex configuration or elevated privileges (though apf-auth might still be necessary).
• Each unit test case would be written in a separate .c unit—containing an extern entry function and several static functions—and compiled into the executable.
• Each test case would be selectable by a unique name and run independently, with no inter-test dependencies allowed.
• The framework would also provide a shared utility module accessible to all test units.
• To keep things simple, it would be compiled and executed solely within the z/OS environment.
• CMS-related components could be ignored for the time being.
• Its execution method could mirror that of the regression testing framework.

This is just a concept for now, and I’m not yet certain if implementation is necessary, but I believe it is technically feasible.

[ChongZhou-Broadcom]

Can you turn the "hints for testing" into tests that are part of this PR. The scripts should go in a subdir of zowe-common-c/tests that you create and one script should build the directory structure that can reproduce the problem, another can compile a tiny program to run the test. That would allow me show that the bug existed and was cleared by this fix by running the test in a local repo that does NOT have this fix in it yet.

Status of this PR: I am creating the test requested by Joe. It will be ready this week.

[ChongZhou-Broadcom]

@JoeNemo Per your request, a runnable unit test program is added into the tests/zosfiles directory.

This PR contains many updates and corrections to the zosfile.c so I will briefly summarize the major changes/corrections:

1. Function fileCreateSymlink was added for the directory copying function. The previous implementation did not correctly handle symbolic links.
2. Function directoryListEntries and getEntryName were added for all functions that works with an entry list of the target directory. The previous implementation repeats the same entry listing logic.
3. Corrected getValidDirectoryEntries function. The previous implementation returns non-null-terminated strings but the callers assumed the returned strings are null-terminated, which may cause buffer overrun. (already happened in my tests)
4. Function fileReadLink2 was added for all directory-recursion functions. It is sort of a combination of symbolicFileInfo + fileReadLink. It tells if a path is a real file, a real directory, or a symbolic link, and returns the real path for symbolic links.
5. Corrected directoryCopy function. The previous implementation followed symbolic links to directories and made full copies of them, but it is obviously not a good behavior and it differs from the copy -R command. The new implementation only recreates symbolic links.
6. Corrected directoryDeleteRecursive function. The reason is similar to directoryCopy. The new implementation unlink symbolic links instead of deleting the source files.
7. Added a runnable unit test program in the tests/zosfiles directory.

[JoeNemo]

Re-tested the current head (2432356) on z/OS (Marist):

• tests/zosfiles: make, then ./prepare_test.sh | xargs ./zosfiles -> Done. Total: 6. Failed:0. (the owner case skips without ZOSFILES_UT_USERID/GRPID, as designed).
• My own canary test from July, which failed on the original commit: a tree with a symlink to a file outside it. fileReadLink2 now reports SYMLINK, directoryCopy recreates the link instead of copying the target, and directoryDeleteRecursive removes the tree with the target untouched. All pass. The 29 July change to read entry names by length fixed it.

Two things stand between this and a merge:

1. The branch conflicts with v3.x/staging on CHANGELOG.md only.
2. The SonarCloud gate fails on one reliability finding at zosfile.c ~1220: the METTLE trace in directoryClose passes returnValue twice, so the arguments are shifted against the format. Pre-existing, but the PR touched those lines so it counts as new. Dropping the duplicate returnValue, clears it.

[JoeNemo]

There are zowelog lines in zosfiles directoryClose() that duplicate "returnValue". Looks like a typo.

[sonarqubecloud]

Quality Gate failed

Failed conditions
C Reliability Rating on New Code (required ≥ A)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE

[ChongZhou-Broadcom]

@JoeNemo Thanks for the feedbacks. The conflicts is resolved (I rebased on the latest 3.x/staging and resolved conflicts). The redundant returnValue is removed.

[JoeNemo]

Approved.