Windows support #510

Piloalucard · 2025-04-18T04:32:41Z

Summary of changes

This pull request includes following changes or fixes.

Compatible code with Microsoft Visual C++ Compiler
Support of Windows Server 2022
Support of Windows Server 2025
Support of Windows 11

Description

Code was updated to be compatible again with Windows platform. Microsoft mandates compilation with Microsoft tools to give support, with that said the code needed to be compatible with Microsoft Visual C++ Compiler for a new version to be built.
Macros we're added to comply with this and change the behavior depending on which platform the project is being built.

Type of change

Refactoring
Breaking change (fix or feature that would cause existing functionality to not work as expected)
This change requires a documentation update

Checklist:

My code follows the style guidelines of this project
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
My changes generate no new warnings
I have confirmed my fix is effective or that my feature works

Author: Author Name vandel_van@hotmail.com

…ndows

piste-jp · 2025-04-21T01:47:50Z

I would like to notify this comment in advance before this PR goes to compilable...

I know you are working so hard on this PR and also I know you want to support windows again in the IBM storage archive product. But I must strongly disagree to this PR. Because this PR has no benefit to the normal open source users who are using the code in this repository. On the other hand code change is big and impact of the readability of codes are really big.

I have so many comments and complains but top-3 of my comments are

First, please don't introduce crossbuild directory easily. This kind of architecture specific code is handled under libltfs/arch directory. Please follow this as much as possible.

Second, please don't use SAFE_ prefix in the macros in commons.h. Frankly I cannot understand what does SAFE_ means. I believe they are "safe" only in windows environment for for Linux/Mac/BSDs env. I don't recommend that we use the C++11/C99 "Annex K" functions because of its history, they are optional and they are not supported in the glibc yet. What is the problem that LTFS uses current ANSI C standard functions on windows env? I strongly recommend that you need arch_ prefix and design glue layer carefully like thread handling in LTFS, see src/libltfs/ltfs_thread.h carefully.

For example, SAFE_FOPEN looks only safe for windows. And SAFE_WRITE cannot be understand what is "safe" at all.

Last, please write macros to keep the C language context. I believe macro feature can easily ruins the C language context and create new your own langage. But this project is clearly written by C please do not corrupt the readability for C developers. I strongly recommend that you need to use inline function as much possible, that is a reason taht we choose C99.

For example, this macro call confuses C developers in short period. They don't think conf_file is output of function (macro) at a glance. Of course, they can understand soon, but that small thinking time reduces the readability totally.

SAFE_FOPEN(path, "rb", conf_file);

This reverts commit 092238d.

piste-jp

Why do you want to use arch_ prefix for POSIX functions.

I believe your change doesn't make this code compilable on Visual C. This just a part of Visual C compilation only in IBM.

So I believe you need to minimize the code change because there is no benefit to this project at all!

My recommendation is just use POSIX functions as POSIX function name.

Honestly, I cannot understand why you want to replace POSIX function name to name with arch_ prefix. But if it is to suppress the waring message in Visual C, please suppress it with pragma declaration like below.

#ifdef _MSC_VER
#pragma warning(suppress : 4996)
...
#endif

See also, https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/posix-write?view=msvc-170.

Second best is

#ifdef _MSC_VER
#define write _write
...
#endif

src/utils/ltfsck.c

piste-jp · 2025-04-30T01:54:14Z

src/libltfs/arch/ltfs_arch_ops.h

+#define INVALID_KEY UINT_MAX
+
+
+    #define arch_vsprintf(buffer,bufferCount, fmt, ...) vsprintf_s((buffer), (bufferCount), (fmt), __VA_ARGS__)


Please do not use camel case bufferCount.

This is macro definition, so long variable name is not needed.

piste-jp

I roughly make comments. They are not everything.

I just at a glance and make comments.

piste-jp · 2025-04-30T02:04:38Z

src/iosched/unified.c


 	/* Store new write request */
-	new_req = calloc(1, sizeof(struct write_request));
+	new_req = (struct write_request*)calloc(1, sizeof(struct write_request));


I'm not sure why this cast is needed.

piste-jp · 2025-04-30T02:05:28Z

src/iosched/unified.c

 	struct timer_info timerinfo;
 	struct unified_data *priv = iosched_handle;
-
+	


Do not keep the leading spaces in an empty line.

piste-jp · 2025-04-30T02:08:45Z

src/iosched/unified.c

 		}

-		p = fopen(path, PROFILER_FILE_MODE);
+		arch_fopen(path, PROFILER_FILE_MODE,p);


why fopen_s() is needed on windows?

piste-jp · 2025-04-30T02:14:29Z

src/kmi/simple.c

-		strcat((char *) priv.dk_list, (char *) key[i].dk);
-		strcat((char *) priv.dk_list, ":");
-		strcat((char *) priv.dk_list, (char *) key[i].dki);
+			arch_strcat((char *) priv.dk_list, dk_list_len, "/");


I cannot　understand why strcat_s() is needed here. Every input string can be handled safely in this function...

If you have a reason please explain it by comment and add strcat_s() function into linux side. I don't like Annex K, but it is better than this, if you have a strong reason.

piste-jp · 2025-04-30T02:16:38Z

src/libltfs/arch/ltfs_arch_ops.h

+
+    #define arch_access(filename, mode) _access((filename), (mode))
+
+    #define arch_xmlfree(ptr) xmlFree((ptr))


I recommend this.

#define xmlfree xmlFree

piste-jp · 2025-04-30T02:17:19Z

src/libltfs/arch/ltfs_arch_ops.h

+
+    #define arch_access(filename, mode) access(filename, mode)
+
+    #define arch_xmlfree(ptr) arch_safe_free(ptr)


I recommend that this line is removed.

piste-jp · 2025-04-30T02:24:31Z

src/libltfs/index_criteria.c

-	char rule[len+1], last, *ptr;
-
+	char *rule = NULL, last, *ptr;
+	int ruleLen = (sizeof(char) * (int)(len + 1));


Please do not use camel case.

piste-jp · 2025-04-30T02:25:20Z

src/libltfs/index_criteria.c

+	int ruleLen = (sizeof(char) * (int)(len + 1));
+	rule = (char*)calloc(ruleLen,sizeof(char));
+	if (rule == NULL) {
+		return -LTFS_NO_MEMORY;


Error message is needed.

piste-jp · 2025-04-30T02:26:16Z

src/libltfs/index_criteria.c

+	rulebuf = (char*)malloc(sizeof(char) * (len + 1));
+	if (rulebuf == NULL)
+	{
+		return -LTFS_NO_MEMORY;


Error message is needed.

piste-jp · 2025-05-01T07:30:45Z

Hmm, this PR has so many changes but no benefit to the original code users. Because they don't have any code built on windows even if your change is committed.

Let me clarify how to progress.. I strongly recommend that you start over from scratch again.

Objective

Change the code to be built on Visual C for the IBM product. But the tree cannot be built for this project.

Directions

Keep original code as much as possible, minimize changes
Do not make any refactoring and other prettifications
Drop MinGW support (move to Visual C support)

Changes Detail

Use POSIX functions as is (suppress warnings with #pragma as much as possible for windows)
Do not use Annex K functions as little s possible as (suppress warnings with #pragma as much as possible for windows)
Use inline wrapper functions for above if it is really needed

chukero

Approved

piste-jp

As I said multiple times, I cannot approve this at all.

Number of commits are too many, please reduce number of commits . I cannot review the codes because GitHub is too slow because of excessive number of commits.
Please minimize the changes of codes, please keep the original code as much as possible. If you want to remove warning and to make a refactoring, separate it from this PR.
Please do not replace POSIX functions as arch_ prefixed functions. Windows is a POSIX approved OS, so there is no reason to rename this.
Please implement annex K functions on linux/Mac size if you really need to use them.

piste-jp · 2025-05-01T07:12:13Z

src/libltfs/arch/ltfs_arch_ops.h

+    #define arch_unlink     unlink
+
+    #define arch_write      write
+
+    #define arch_close      close
+
+    #define arch_read       read
+
+    #define arch_strdup     strdup
+
+    #define arch_chmod      chmod
+
+    #define arch_getpid     getpid
+
+    #define arch_access     access


Hmm, Is arch_ prefix needed for POSIX functions ?

Remove lines in this #else block.

piste-jp · 2025-05-01T07:15:21Z

src/libltfs/arch/ltfs_arch_ops.h

+    #define arch_unlink     _unlink
+
+    #define arch_write      _write
+
+    #define arch_close      _close
+
+    #define arch_read       _read
+
+    #define arch_strdup     _strdup
+
+    #define arch_chmod      _chmod
+
+    #define arch_getpid     _getpid
+
+    #define arch_access     _access


Hmm, is arch_ prefix needed for POSIX functions?

Why not to use the code below.

#define unlink _unlink #define write _write #define close _close #define read _read #define strdup _strdup #define chmod _chmod #define getpid _getpid #define access _access

Piloalucard added 5 commits April 17, 2025 18:07

Cherry pick from build-windows

184d916

Cherry pick from build-windows

f3a4e82

Update make_message_src.sh

3bd0ba0

Undoing unwanted changes and comments

8b88246

Update compatibility.h

81b2ff4

Piloalucard requested review from amissael95, chukero, juliocelon, rangelmx, syaoraang and vandelvan April 18, 2025 04:32

Piloalucard self-assigned this Apr 18, 2025

Corrections for Linux build with windows changes

15e69c6

vandelvan force-pushed the windows branch 4 times, most recently from bafbd71 to 07a2da6 Compare April 18, 2025 23:26

Corrections for Linux build with windows changes

a7a2591

Author: Author Name vandel_van@hotmail.com

vandelvan force-pushed the windows branch from 07a2da6 to a7a2591 Compare April 18, 2025 23:29

Merge branch 'windows' of https://github.com/Piloalucard/ltfs into wi…

80395fa

…ndows

Piloalucard and others added 9 commits April 24, 2025 11:47

Changed macros to arch ops

9b03b94

rewriting functions

f2ed251

corrections for Linux build

c8c70bf

Making non-static for linux

46488e1

Update pathname.c

c49d6b6

Update pathname.c

b68e771

Update ltfs_arch_ops.h

ba335dc

Merge branch 'v2.4-stable' into windows

2ef6dec

Update ltfs_arch_ops.h

ebbb858

vandelvan added 3 commits April 28, 2025 15:54

updated python2 to python3 for validate error messages

092238d

Revert "updated python2 to python3 for validate error messages"

14a7620

This reverts commit 092238d.

fix: warnings during build in linux

757a2ed

piste-jp requested changes Apr 30, 2025

View reviewed changes

piste-jp reviewed Apr 30, 2025

View reviewed changes

Piloalucard added 4 commits April 30, 2025 08:55

Asterisk fix in pointers decl to match standard

83f8380

Update ltfs_arch_ops.h

35760f9

Update ltfs_arch_ops.h

45662c0

Updated arch_ops and added missing error msgs

961c2b9

vandelvan removed their request for review May 2, 2025 17:43

Piloalucard added 6 commits May 3, 2025 20:23

build errors

f50ad91

Update filedebug_tc.c

5eb697e

Copyright not used

08e798b

Update ltfsck.c

f752cc8

missing correct cast

9d961d3

Removing unnecesary changes

5bfc706

chukero approved these changes May 5, 2025

View reviewed changes

vandelvan self-assigned this May 5, 2025

piste-jp requested changes May 6, 2025

View reviewed changes

Piloalucard closed this May 7, 2025

piste-jp mentioned this pull request May 8, 2025

Windows support #520

Merged

6 tasks

		#define INVALID_KEY UINT_MAX


		#define arch_vsprintf(buffer,bufferCount, fmt, ...) vsprintf_s((buffer), (bufferCount), (fmt), __VA_ARGS__)

		struct timer_info timerinfo;
		struct unified_data *priv = iosched_handle;


		#define arch_access(filename, mode) _access((filename), (mode))

		#define arch_xmlfree(ptr) xmlFree((ptr))


		#define arch_access(filename, mode) access(filename, mode)

		#define arch_xmlfree(ptr) arch_safe_free(ptr)

Windows support #510

Windows support #510

Uh oh!

Conversation

Piloalucard commented Apr 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary of changes

Description

Type of change

Checklist:

Uh oh!

piste-jp commented Apr 21, 2025

Uh oh!

piste-jp 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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

piste-jp 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!

piste-jp commented May 1, 2025

Objective

Directions

Changes Detail

Uh oh!

chukero left a comment

Choose a reason for hiding this comment

Uh oh!

piste-jp 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Piloalucard commented Apr 18, 2025 •

edited

Loading

piste-jp left a comment •

edited

Loading