Add Windows support to OpenZFS #14034
Conversation
|
Hey this is amazing, I was just thinking a few hours ago how awesome it would be if openzfs for windows started integrating with this repo. I was also thinking looking at this PR, have you heard of cccl? I've tried this with autotools and it works amazingly well. I could work on adding this support to the existing autotools system once this PR stabilizes. Would you be in favor of that? Not as a replacement for cmake, but as an extension to the existing config system. |
|
|
|
This made me realize that the amount of code duplication we have between platforms is ridiculous. That should be cleaned up (although it does not block this PR). I know this is a draft, but I could not help peek at it and made a few comments on things that stood out to me. |
|
It's amusing, I'm pleased how little code duplication there is, now. Compared to when we did the first mac port :) Thanks for glancing at it. The death after bookmark/setup test is suspicious, so that is next to look into. |
| @@ -40,7 +40,6 @@ | |||
| #include <libintl.h> | |||
| #include <stdio.h> | |||
| #include <stdlib.h> | |||
| #include <strings.h> | |||
There was a problem hiding this comment.
the "remove strings.h" missed a few, which could be its own PR
| @@ -719,6 +730,14 @@ zfs_prop_init(void) | |||
| PROP_READONLY, ZFS_TYPE_DATASET, "KEYGUID", B_TRUE, sfeatures); | |||
| zprop_register_hidden(ZFS_PROP_REDACTED, "redacted", PROP_TYPE_NUMBER, | |||
| PROP_READONLY, ZFS_TYPE_DATASET, "REDACTED", B_FALSE, sfeatures); | |||
| #ifdef _WIN32 | |||
| zprop_register_string(ZFS_PROP_DRIVELETTER, "driveletter", "-", | |||
There was a problem hiding this comment.
PROPs must always be defined on all platforms, remove #ifdef
cmd/zpool/zpool_main.c
Outdated
|
|
||
| #ifdef _WIN32 | ||
| #include <termios.h> | ||
| #endif | ||
| #include "zpool_util.h" |
There was a problem hiding this comment.
Can we add here a new line again. I think, it was there because of: <system-stuff.h> and "own-stuff.h"
contrib/bpftrace/Makefile.am
Outdated
| @@ -1,3 +1,3 @@ | |||
| dist_noinst_DATA += %D%/taskqlatency.bt %D%/zfs-trace.sh | |||
|
|
|||
| SHELLCHECKSCRIPTS += %D%/zfs-trace.sh | |||
| SHELLCHECKSCRIPTS += %D%/zfs-trace.sh | |||
There was a problem hiding this comment.
There should be a new line ?
lundman
force-pushed
the
windows
branch
3 times, most recently
from
5eed807 to
8406c93
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| LOGGER_SESSION, OPEN_ZFS_GUID, flags.c_str(), | ||
| levels.c_str(), size_in_mb, etl_file.c_str()); | ||
|
|
||
| ret = system(command); |
Check failure
Code scanning / CodeQL
Uncontrolled data used in OS command
There was a problem hiding this comment.
I had originally looked at this from my ipad and could not see the context due to the iPad being slow, so I made a (now deleted) remark that I now see was wrong.
Anyway, the reason that CodeQL complains here is that command injection is possible by altering the arguments passed to the installer and in general, command injection is something to be avoided whenever possible. Here are some references:
https://codeql.github.com/codeql-query-help/cpp/cpp-command-line-injection/
https://wiki.sei.cmu.edu/confluence/display/c/STR02-C.+Sanitize+data+passed+to+complex+subsystems
That said, I do not know the right way to sanitize this offhand, but the whitelist example by Wietse Venema at the second URL looks like a sane solution.
lundman
force-pushed
the
windows
branch
6 times, most recently
from
766f5f1 to
cc80ea4
Compare
include/zfs_fletcher.h
Outdated
| NTSTATUS saveStatus; | ||
| XSTATE_SAVE SaveState; | ||
| #endif | ||
|
|
There was a problem hiding this comment.
I would prefer it if we put these into zio_abd_checksum_data_t and put boolean_t call_kfpu; into fletcher_4_ops_t. The thing that I dislike about changing fletcher_4_ctx to a struct and passing it to kfpu_begin_ctx() and kfpu_end_ctx(). ties those functions to fletcher_4_ctx, when they are general purpose functions that should be usable for far more than fletcher4.
I guess another way would be to put these members before the anonymous union, make a structure for these members and use a void pointer to do casting, but I like your boolean_t call_kfpu; idea from the other day.
There was a problem hiding this comment.
This is just a stop-gap until, hopefully, the new implementation @AttilaFueloep is working on merges. It is a separate commit, so I can easily drop it when that time comes.
There was a problem hiding this comment.
So there seems to be agreement that the refactoring I suggested yesterday is a good idea? If so, I'll prepare a PR tomorrow.
There was a problem hiding this comment.
If my vote counts, I'm all for it.
Starting to think it doesn't know |
There was a problem hiding this comment.
I notice that time_t is 64-bit on 64-bit UNIX systems because long is 64-bit on them, but due to Windows' memory model, long is 32-bit while time_t is 64-bit. The standard has a hole here, since inttypes.h, which should give a way to print this in a sane way, does not. Something like this would probably work:
#ifdef _ILP32
#define PRI_TIME PRId32
#else
#define PRI_TIME PRId64
#endif
(void) printf("%s\t%s\t" PRI_TIME "\n", zname, tagname, time);
We have a similar, but subtly different issue with uid_t, where Linux and presumably other UNIX systems use unsigned int, while Windows appears to use unsigned long long. Interestingly, this reveals a case where the type is not quite right, because %d is used instead of %u.
That said, doing a 64-bit uid_t with the works fine on little endian systems as long as the uid_t being passed is the last argument passed and the top 32-bits are not actually used. If it is not, then the next argument will be considered to start in the upper half of the passed uid_t. On a big endian system, this will pass the top 32-bits of the uid_t, which is wrong, although Windows has not supported big endian in years, so that is less of a concern than the former.
We could solve this similarly to how we can solve the time_t issue, by doing:
#if defined(_ILP32) || !defined(_WIN32)
#define PRI_UID PRIu32
#else
#define PRI_UID PRIu64
#endif
#endif
(void) sprintf(id, PRI_UID, rid);
| cp = g->must; | ||
| scan = start; | ||
| memset(&mbs, 0, sizeof (mbs)); | ||
| while (cp < g->must + g->mlen) { |
There was a problem hiding this comment.
This is a false alarm because memset() is not being used here for security reasons, but it would be nice not to have an alert. While github code scanning will not show this on future PRs after this is merged, it will still appear in the security tab in repositories where we have write access. It is possible to mark it as a false positive there, but it needs to be done for each individual repository and I would rather not have people get into the habit of marking alerts as false positives if it is not necessary. In this case, we could add an #ifdef NLS to suppress the alert from memset().
| LOGGER_SESSION, OPEN_ZFS_GUID, flags.c_str(), | ||
| levels.c_str(), size_in_mb, etl_file.c_str()); | ||
|
|
||
| ret = system(command); |
There was a problem hiding this comment.
I had originally looked at this from my ipad and could not see the context due to the iPad being slow, so I made a (now deleted) remark that I now see was wrong.
Anyway, the reason that CodeQL complains here is that command injection is possible by altering the arguments passed to the installer and in general, command injection is something to be avoided whenever possible. Here are some references:
https://codeql.github.com/codeql-query-help/cpp/cpp-command-line-injection/
https://wiki.sei.cmu.edu/confluence/display/c/STR02-C.+Sanitize+data+passed+to+complex+subsystems
That said, I do not know the right way to sanitize this offhand, but the whitelist example by Wietse Venema at the second URL looks like a sane solution.
lundman
force-pushed
the
windows
branch
2 times, most recently
from
0019403 to
0efb36e
Compare
| unsigned size; | ||
| { | ||
| (void)opaque; | ||
| return sizeof(uInt) > 2 ? (voidpf)malloc(items * size) : |
There was a problem hiding this comment.
A typecast to (size_t) is needed on size. Otherwise, you can under-allocate memory.
In practice, this probably does not happen at the sizes we allocate, but it is better to be safe than sorry. A patch fixing this is likely needed by upstream zlib, since there is a clear memory safety issue here.
| lhp->lh_nchunks = nchunks; | ||
| lhp->lh_chunksize = P2ROUNDUP(logsize / nchunks + 1, PAGESIZE); | ||
| lhp->lh_base = vmem_alloc_impl(kmem_log_arena, | ||
| lhp->lh_chunksize * nchunks, VM_SLEEP); |
There was a problem hiding this comment.
Although we should not be doing allocations that are >=4GB in size, if we ever do, this will underallocate. It should be given a typecast to (size_t) as a defensive measure to harden against future changes that will try to allocate big things.
| percent = (used * 100) / kmem_dump_size; | ||
|
|
||
| kmem_dumppr(&p, e, "%% heap used,%d\n", percent); | ||
| kmem_dumppr(&p, e, "used bytes,%ld\n", used); |
There was a problem hiding this comment.
size_t should change size based on the machine word size, but I am not aware of a printf specifier that does on Windows. We probably will need this in a header somewhere:
#ifdef _ILP32
#define PRI_SIZE "%lu"
#define PRI_SSIZE "%ld"
#else
#define PRI_SIZE "%llu"
#define PRI_SSIZE "%lld"
#endif
Then we could make this:
kmem_dumppr(&p, e, "used bytes," PRI_SIZE "\n", used);
There was a problem hiding this comment.
This will cause incorrect numbers to be printed whenever bit 32 or greater is used.
|
|
||
| kmem_dumppr(&p, e, "%% heap used,%d\n", percent); | ||
| kmem_dumppr(&p, e, "used bytes,%ld\n", used); | ||
| kmem_dumppr(&p, e, "heap size,%ld\n", kmem_dump_size); |
| while (reap-- && | ||
| (mp = kmem_depot_alloc(cp, &cp->cache_full)) != NULL) { | ||
| kmem_magazine_destroy(cp, mp, cp->cache_magtype->mt_magsize); | ||
| bytes += cp->cache_magtype->mt_magsize * cp->cache_bufsize; |
There was a problem hiding this comment.
If the types of these are not widened, we need a defensive (size_t) cast.
| if (IS_P2ALIGNED(cache_size, PAGESIZE)) | ||
| align = PAGESIZE; | ||
| (void) snprintf(name, sizeof (name), | ||
| "kmem_alloc_%lu", cache_size); |
There was a problem hiding this comment.
Provided that you take my previous suggestion to make PRI_SIZE, we can use that here.
|
There are many failures in the Linux ZTS tests that need to be diagnosed and fixed. A normal run gives:
The runs being done on this PR give:
|
I will at very least look at the 7 // Pfft, cosmetics. |
lundman
force-pushed
the
windows
branch
2 times, most recently
from
2eaaadf to
0ce5a1c
Compare
|
This is interesting. CentOS 7:
CentOS 8:
|
lundman
force-pushed
the
windows
branch
3 times, most recently
from
db7af11 to
ceb4324
Compare
|
Oh yeah, promoted PR, now all grown up. |
Signed-off-by: Jorgen Lundman <lundman@lundman.net>
Signed-off-by: Jorgen Lundman <lundman@lundman.net>
For the variabe defined inside kfpu_begin(). Use kfpu_begin_ctx() for the split init/fini functions. Signed-off-by: Jorgen Lundman <lundman@lundman.net>
|
This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation. |
Motivation and Context
This PR contains the required changed to add Windows as a platform, if that
is desirable.
Description
Windows uses CMake environment, and does not touch any of the Unix build system.
How Has This Been Tested?
Types of changes
Checklist:
Signed-off-by.