Add saunafs command on Windows #298

rolysr · 2025-02-12T14:35:01Z

This PR is related to the corresponding integration of the saunafs tool on a Windows environment. The main changes were:

Fix related dependencies and added Windows specific code to make the tools submodule to be compiled on Windows.
Fix the open_master_conn function in order to correctly get the corresponding inode from a given Windows file system path.
Add Windows specific code for making the commands makesnapshot and rremove to work on Windows.

For making the saunafs command available on Windows, this commit adds the necessary changes to compile the saunafs command on Windows. Signed-off-by: rolysr <[email protected]>

The `open_master_conn` function was not working properly on Windows when using it with the saunafs tool because it was not properly resolving the inode by a given path on the file system. This commit fixes that issue. Signed-off-by: rolysr <[email protected]>

The makesnapshot command was failing on saunafs tool on Windows because of not correctly resolving the source inode from which the snapshot was going to be created. This commit fixes that issue. Signed-off-by: rolysr <[email protected]>

The previous implementation of recursive remove was not correctly delimiting the path separator for a parent path from a given full path when using the saunafs tool on Windows. This commit fixes that issue. Signed-off-by: rolysr <[email protected]>

dmga44

Great work! 👍

As a general comment, I think the changes would have looked better if they were ordered like:

enough to be able to compile in Windows
fixes on open_master_conn function, including the changes in the protocol.
fixes on each of the tools, single commit or multiple commits, as you see fit.
Though it would be a chore to make it that way now.

dmga44 · 2025-02-13T01:22:44Z

src/common/quota_database.h

I think you should also move the file quota_database.cc file to make the change more consistent.

dmga44 · 2025-02-13T01:23:18Z

src/common/signal_handling.h

+
+#else
+#include <signal.h>
+#endif


Please notice the missing endline.

dmga44 · 2025-02-13T01:37:56Z

src/master/filesystem_quota.h

@@ -25,6 +25,8 @@
 #include <utility>
 #include "protocol/quota.h"
 #include <master/filesystem_node_types.h>
+#include "common/quota_database.h"
+#include "master/filesystem_freenode.h"


I think we should come to an agreement on whether to use quotes or angular in these kind of includes.

dmga44 · 2025-02-13T02:07:22Z

src/master/filesystem_operations.cc

+	inode = current_inode;
+	return SAUNAFS_STATUS_OK;
+}
+


I think there are some permission checks missing here.

You could think of this function as several fs_lookup calls one on top of the other. There are things which are there that are not present here, to mention some:

call to verify_session

call to fsnodes_get_node_for_operation (I think this is the one doing the permission check)

checks on the implicit entries (. and ..)

call to fsnodes_namecheck

entry in gFsStatsArray and increasing its count.

Consider the approach to partition the path and call severall fs_lookup (ignoring the gFsStatsArray update).

Also notice that some of these add-ups could be applied in fs_full_path_by_inode.

dmga44 · 2025-02-13T02:12:24Z

src/mount/client/saunafs_c_api.cc

+		uint32_t uid = getuid();
+		uint32_t gid = getgid();
+#endif
+		Client::Context *ret = new Client::Context(uid, gid,


Is this change required? I don't think we are building the this client in Windows.

dmga44 · 2025-02-13T03:06:56Z

src/tools/snapshot.cc

+		int srcname_len = strlen(srcnames[i]);
+		if (srcnames[i][srcname_len - 1] == '/' ||
+		    srcnames[i][srcname_len - 1] == '\\') {
+			srcnames[i][srcname_len - 1] = 0;


Consider writing
srcnames[i][srcname_len - 1] = '\0';
to make clearer you are setting the end of the string.

dmga44 · 2025-02-13T03:16:16Z

src/tools/snapshot.cc

+#ifdef _WIN32
+						if (GetFullPathName(srcnames[i], PATH_MAX, src, NULL) ==
+						    0) {
+							printf("%s: GetFullPathName error: %lu\n",
+							       srcnames[i], GetLastError());
+							return -1;
+						}
+#else
 						if (realpath(srcnames[i], src) == NULL) {
 							printf("%s: realpath error on %s: %s\n", srcnames[i], src,
 							       strerr(errno));
 							status = -1;
 							continue;
 						}
+#endif


Is it correct that the Windows code is directly returning while the Linux analogous one is setting up status and continuing?

dmga44 · 2025-02-13T03:17:03Z

src/tools/snapshot.cc

+#ifdef _WIN32
+						if (GetFullPathName(srcnames[i], PATH_MAX, src, NULL) ==
+						    0) {
+							printf("%s: GetFullPathName error: %lu\n",
+							       srcnames[i], GetLastError());
+							return -1;
+						}
+#else
 						if (realpath(srcnames[i], src) == NULL) {
 							printf("%s: realpath error on %s: %s\n", srcnames[i], src,
 							       strerr(errno));
 							status = -1;
 							continue;
 						}
+#endif


Same concern here.

dmga44 · 2025-02-13T03:23:15Z

src/tools/snapshot.cc

+#ifdef _WIN32
+		if (GetFullPathName(dir, PATH_MAX, to, NULL) == 0) {
+			printf("%s: GetFullPathName error: %lu\n", dir, GetLastError());
+			return -1;
+		}
+#else
 		if (realpath(dir, to) == NULL) {
 			printf("%s: realpath error on %s: %s\n", dir, to, strerr(errno));
 			return -1;
 		}
+#endif


Consider finding a way to try to not repeat code (and preprocessor directives) in the GetFullPathName and realpath consecutive calls.

dmga44 · 2025-02-13T03:25:45Z

src/tools/tools_common_functions.cc

-	if (endp == path) {
+	if ((endp - path) == 2 && path[1] == ':' && path[2] == '\\') {
+		path[3] = '.';
+		path[4] = '\0';


Please consider the case of mountpoints in subfolders.

rolysr requested review from uristdwarf, lgsilva3087, ralcolea, antuan96314, ictus4u and dmga44 February 12, 2025 14:35

rolysr force-pushed the add-saunafs-command-on-windows branch 2 times, most recently from 20157eb to 34eaa8d Compare February 12, 2025 14:40

rolysr added 4 commits February 12, 2025 23:16

feat(tools): Add saunafs command on Windows

fbc0bd6

For making the saunafs command available on Windows, this commit adds the necessary changes to compile the saunafs command on Windows. Signed-off-by: rolysr <[email protected]>

fix(tools): Fix makesnapshot command on Windows

b519256

The makesnapshot command was failing on saunafs tool on Windows because of not correctly resolving the source inode from which the snapshot was going to be created. This commit fixes that issue. Signed-off-by: rolysr <[email protected]>

rolysr force-pushed the add-saunafs-command-on-windows branch from 34eaa8d to add65ff Compare February 12, 2025 22:17

dmga44 reviewed Feb 13, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add saunafs command on Windows #298

Add saunafs command on Windows #298

rolysr commented Feb 12, 2025 •

edited

Loading

dmga44 left a comment

dmga44 Feb 13, 2025

dmga44 Feb 13, 2025

dmga44 Feb 13, 2025

dmga44 Feb 13, 2025

dmga44 Feb 13, 2025

dmga44 Feb 13, 2025

dmga44 Feb 13, 2025

dmga44 Feb 13, 2025

dmga44 Feb 13, 2025

dmga44 Feb 13, 2025

Add saunafs command on Windows #298

Are you sure you want to change the base?

Add saunafs command on Windows #298

Conversation

rolysr commented Feb 12, 2025 • edited Loading

dmga44 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rolysr commented Feb 12, 2025 •

edited

Loading