onepub-dev / dart_posix

MIT License
12 stars 4 forks source link

Mode class isFile, isDirectory etc not working #2

Closed ab36245 closed 2 years ago

ab36245 commented 2 years ago

The file type is* getters (isFile, isDirectory) etc don't seem to work correctly.

I think the problem is the bit masking operations in the getters:

bool get isFile => _mode & _S_IFREG == 1;
bool get isDirectory => _mode & _S_IFDIR == 1;
...

I think the comparison to 1 should be to the mask itself:

bool get isFile => _mode & _S_IFREG == _S_IFREG;
bool get isDirectory => _mode & _S_IFDIR == _S_IFDIR;
...

This is how the other getters for permissions are implemented.

bsutton commented 2 years ago

You are correct.

Would you be able to raise a pr with the changes. Some unit tests for the same would also be really good :)

On Thu, 4 Nov 2021, 11:04 am absynth, @.***> wrote:

The file type is* getters (isFile, isDirectory) etc don't seem to work correctly.

I think the problem is the bit masking operations in the getters:

bool get isFile => _mode & _S_IFREG == 1; bool get isDirectory => _mode & _S_IFDIR == 1; ...

I think the comparison to 1 should be to the mask itself:

bool get isFile => _mode & _S_IFREG == _S_IFREG; bool get isDirectory => _mode & _S_IFDIR == _S_IFDIR; ...

This is how the other getters for permissions are implemented.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/noojee/dart_posix/issues/2, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAG32OBY3BELIDZQKFGJWHLUKHEY5ANCNFSM5HKGTJWQ . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

ab36245 commented 2 years ago

I've started work on a pr, but I think there's more to the problem.

I'm running on macos and I suspect the structure alignment is not correct. The fields returned in _stat_struct seem to be screwy.

I'm not very familiar with dart:ffi. How can I check that a correctly aligned stat() is being called?

bsutton commented 2 years ago

The answer is it's not easy.

Start by reviewing the macos doco for the stat function.

On Thu, 4 Nov 2021, 12:52 pm absynth, @.***> wrote:

I've started work on a pr, but I think there's more to the problem.

I'm running on macos and I suspect the structure alignment is not correct. The fields returned in _stat_struct seem to be screwy.

I'm not very familiar with dart:ffi. How can I check that a correctly aligned stat() is being called?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/noojee/dart_posix/issues/2#issuecomment-960368997, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAG32OBH46JG4D37JYXZ6N3UKHRMPANCNFSM5HKGTJWQ . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

ab36245 commented 2 years ago

Just so I understand, you assume that there is a call on each platform that maps directly into the defined _struct_stat?

If so, I'm beginning to think that may not be possible and that it'll need to have separate native structs which then map to the Stat class.

bsutton commented 2 years ago

Yes that is the current assumption.

I think there are two versions of the stat function. I believe that I was using the more modern version and that it was supported on macos, but that conclusion may be wrong.

S. Brett Sutton Noojee Contact Solutions 03 8320 8100

On Thu, 4 Nov 2021 at 14:23, absynth @.***> wrote:

Just so I understand, you assume that there is a call on each platform that maps directly into the defined _struct_stat?

If so, I'm beginning to think that may not be possible and that it'll need to have separate native structs which then map to the Stat class.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/noojee/dart_posix/issues/2#issuecomment-960424779, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAG32OFBVCQPYKQVZ533TLLUKH4CBANCNFSM5HKGTJWQ . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

ab36245 commented 2 years ago

Thanks, I'll look into it (testing on Debian 11 64 bit now) and pull together a pr for it

ab36245 commented 2 years ago

I think it'll have to be two separate structs. Here is some testing with a simple C program on a 64 bit Debian 11 box (kernel 5.10) and on a Mac (x86_64) running MacOS 12 (Monterey).

The Linux version uses __xstat(1,...) and the mac version uses plain stat(). I can't find any alternative stat for the mac (although I've not done a lot of C programming on a mac so such a beast may exist).

The program stats a simple text file (47 bytes) and prints (most of) the stat structure. Notice the different sizes and consequent offsets. Also note the different orders of the fields and the slightly different field names!

Linux:

st_dev                    offset   0 sizeof  8 = 0x0000000000fe01                65025
st_ino                    offset   8 sizeof  8 = 0x00000000180010              1572880
st_nlink                  offset  16 sizeof  8 = 0x00000000000001                    1
st_mode                   offset  24 sizeof  4 = 0x000000000081a4                33188
st_uid                    offset  28 sizeof  4 = 0000000000000000                    0
st_gid                    offset  32 sizeof  4 = 0000000000000000                    0
st_rdev                   offset  40 sizeof  8 = 0000000000000000                    0
st_size                   offset  48 sizeof  8 = 0x0000000000002f                   47
st_blksize                offset  56 sizeof  8 = 0x00000000001000                 4096
st_blocks                 offset  64 sizeof  8 = 0x00000000000008                    8
st_atim.tv_sec            offset  72 sizeof  8 = 0x0000006183543c           1635996732
st_atim.tv_nsec           offset  80 sizeof  8 = 0x00000037b714d4            934745300
st_mtim.tv_sec            offset  88 sizeof  8 = 0x0000006183541e           1635996702
st_mtim.tv_nsec           offset  96 sizeof  8 = 0x0000002b956989            731212169
st_ctim.tv_sec            offset 104 sizeof  8 = 0x0000006183541e           1635996702
st_ctim.tv_nsec           offset 112 sizeof  8 = 0x0000002b956989            731212169

Mac:

st_dev                    offset   0 sizeof  4 = 0x00000001000004             16777220
st_mode                   offset   4 sizeof  2 = 0x000000000081a4                33188
st_nlink                  offset   6 sizeof  2 = 0x00000000000001                    1
st_ino                    offset   8 sizeof  8 = 0x0000001a3ddb56            440261462
st_uid                    offset  16 sizeof  4 = 0x000000000001f5                  501
st_gid                    offset  20 sizeof  4 = 0x00000000000014                   20
st_rdev                   offset  24 sizeof  4 = 0000000000000000                    0
st_atimespec.tv_sec       offset  32 sizeof  8 = 0x00000061835d2c           1635999020
st_atimespec.tv_nsec      offset  40 sizeof  8 = 0x00000024d4cac0            617925312
st_mtimespec.tv_sec       offset  48 sizeof  8 = 0x00000061835d2c           1635999020
st_mtimespec.tv_nsec      offset  56 sizeof  8 = 0x000000269ef042            647950402
st_ctimespec.tv_sec       offset  64 sizeof  8 = 0x00000061835d2c           1635999020
st_ctimespec.tv_nsec      offset  72 sizeof  8 = 0x000000269ef042            647950402
st_size                   offset  96 sizeof  8 = 0x0000000000002f                   47
st_blocks                 offset 104 sizeof  8 = 0x00000000000008                    8
st_blksize                offset 112 sizeof  4 = 0x00000000001000                 4096

I suggest a _std_stat_struct and a _mac_stat_struct. I dunno much about ffi but I'm happy to have a got at a PR for this if it makes sense.

bsutton commented 2 years ago

does this help? https://developer.apple.com/library/archive/documentation/System/Conceptual/ManPages_iPhoneOS/man2/fstat64.2.html S. Brett Sutton Noojee Contact Solutions 03 8320 8100

On Thu, 4 Nov 2021 at 15:50, absynth @.***> wrote:

I think it'll have to be two separate structs. Here is some testing with a simple C program on a 64 bit Debian 11 box (kernel 5.10) and on a Mac (x86_64) running MacOS 12 (Monterey).

The Linux version uses __xstat(1,...) and the mac version uses plain stat(). I can't find any alternative stat for the mac (although I've not done a lot of C programming on a mac so such a beast may exist).

The program stats a simple text file (47 bytes) and prints (most of) the stat structure. Notice the different sizes and consequent offsets. Also note the different orders of the fields and the slightly different field names!

Linux:

st_dev offset 0 sizeof 8 = 0x0000000000fe01 65025 st_ino offset 8 sizeof 8 = 0x00000000180010 1572880 st_nlink offset 16 sizeof 8 = 0x00000000000001 1 st_mode offset 24 sizeof 4 = 0x000000000081a4 33188 st_uid offset 28 sizeof 4 = 0000000000000000 0 st_gid offset 32 sizeof 4 = 0000000000000000 0 st_rdev offset 40 sizeof 8 = 0000000000000000 0 st_size offset 48 sizeof 8 = 0x0000000000002f 47 st_blksize offset 56 sizeof 8 = 0x00000000001000 4096 st_blocks offset 64 sizeof 8 = 0x00000000000008 8 st_atim.tv_sec offset 72 sizeof 8 = 0x0000006183543c 1635996732 st_atim.tv_nsec offset 80 sizeof 8 = 0x00000037b714d4 934745300 st_mtim.tv_sec offset 88 sizeof 8 = 0x0000006183541e 1635996702 st_mtim.tv_nsec offset 96 sizeof 8 = 0x0000002b956989 731212169 st_ctim.tv_sec offset 104 sizeof 8 = 0x0000006183541e 1635996702 st_ctim.tv_nsec offset 112 sizeof 8 = 0x0000002b956989 731212169

Mac:

st_dev offset 0 sizeof 4 = 0x00000001000004 16777220 st_mode offset 4 sizeof 2 = 0x000000000081a4 33188 st_nlink offset 6 sizeof 2 = 0x00000000000001 1 st_ino offset 8 sizeof 8 = 0x0000001a3ddb56 440261462 st_uid offset 16 sizeof 4 = 0x000000000001f5 501 st_gid offset 20 sizeof 4 = 0x00000000000014 20 st_rdev offset 24 sizeof 4 = 0000000000000000 0 st_atimespec.tv_sec offset 32 sizeof 8 = 0x00000061835d2c 1635999020 st_atimespec.tv_nsec offset 40 sizeof 8 = 0x00000024d4cac0 617925312 st_mtimespec.tv_sec offset 48 sizeof 8 = 0x00000061835d2c 1635999020 st_mtimespec.tv_nsec offset 56 sizeof 8 = 0x000000269ef042 647950402 st_ctimespec.tv_sec offset 64 sizeof 8 = 0x00000061835d2c 1635999020 st_ctimespec.tv_nsec offset 72 sizeof 8 = 0x000000269ef042 647950402 st_size offset 96 sizeof 8 = 0x0000000000002f 47 st_blocks offset 104 sizeof 8 = 0x00000000000008 8 st_blksize offset 112 sizeof 4 = 0x00000000001000 4096

I suggest a _std_stat_struct and a _mac_stat_struct. I dunno much about ffi but I'm happy to have a got at a PR for this if it makes sense.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/noojee/dart_posix/issues/2#issuecomment-960457057, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAG32OEA2HEUQXYVOKMRB6DUKIGHXANCNFSM5HKGTJWQ . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

ab36245 commented 2 years ago

Yeah, I'd seen that. It's a good thought but there are two problems:

First, the compiler complains that stat64 is deprecated:

cc     x.c   -o x
x.c:16:9: warning: 'stat64' is deprecated: first deprecated in macOS 10.6 [-Wdeprecated-declarations]
    if (stat64(name, &x) < 0) {
        ^
/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk/usr/include/sys/stat.h:427:9: note: 'stat64' has been explicitly marked deprecated here
int     stat64(const char *, struct stat64 *) __OSX_AVAILABLE_BUT_DEPRECATED(__MAC_10_5, __MAC_10_6, __IPHONE_NA, __IPHONE_NA);
        ^
1 warning generated.

which doesn't fill me with a lot of confidence.

Second, the results are the same as the plain stat!

bsutton commented 2 years ago

doing some reading it looks like stat is the correct method.

S. Brett Sutton Noojee Contact Solutions 03 8320 8100

On Thu, 4 Nov 2021 at 16:12, absynth @.***> wrote:

Yeah, I'd seen that. It's a good thought but there are two problems:

First, the compiler complains that stat64 is deprecated:

cc x.c -o x x.c:16:9: warning: 'stat64' is deprecated: first deprecated in macOS 10.6 [-Wdeprecated-declarations] if (stat64(name, &x) < 0) { ^ /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk/usr/include/sys/stat.h:427:9: note: 'stat64' has been explicitly marked deprecated here int stat64(const char , struct stat64 ) OSX_AVAILABLE_BUT_DEPRECATED(MAC_10_5, MAC_10_6, __IPHONE_NA, IPHONE_NA); ^ 1 warning generated.

which doesn't fill me with a lot of confidence.

Second, the results are the same as the plain stat!

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/noojee/dart_posix/issues/2#issuecomment-960464673, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAG32OA6W645CFI3V7QJXPLUKII5VANCNFSM5HKGTJWQ . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

ab36245 commented 2 years ago

There's a lot of confusion about changing to 64 bit inode numbers and the flow-on effect of that but I agree that plain old stat() looks right.

bsutton commented 2 years ago

Did you get anywhere solving this for macos?

ab36245 commented 2 years ago

I've made some progress but it's a bit bigger job than I first expected and I don't have enough time for the next couple of weeks to close it out. I'm very happy to proceed with a PR after that though...

There are two problems.

The main one is that the struct stat call is simply doesn't align with the linux one, so two separate struct definitions are required. I've got the correctly aligned structure going by hand-crafting it. Do you have a preferred way to get the structs? I've dabbled with creating a mac-specific ffigen config file but it generates huge output. I'm guessing you did that for linux and then copied the generated stat struct or is the process somehow more automated?

The second problem is that you have to magically know that the correct version of the system call is called 'stat$INODE64'. Again ffigen helps with that but it's still a copy & paste job basically.

While we're at it I assume you don't have any objection to me adding in lstat at the same time?

bsutton commented 2 years ago

So you are on the right path.

Yes I generated the code and then just picked what I wanted.

Lstat would be a nice edition.

Thanks for the work so far.

On Sun, 14 Nov 2021, 1:29 pm absynth, @.***> wrote:

I've made some progress but it's a bit bigger job than I first expected and I don't have enough time for the next couple of weeks to close it out. I'm very happy to proceed with a PR after that though...

There are two problems.

The main one is that the struct stat call is simply doesn't align with the linux one, so two separate struct definitions are required. I've got the correctly aligned structure going by hand-crafting it. Do you have a preferred way to get the structs? I've dabbled with creating a mac-specific ffigen config file but it generates huge output. I'm guessing you did that for linux and then copied the generated stat struct or is the process somehow more automated?

The second problem is that you have to magically know that the correct version of the system call is called 'stat$INODE64'. Again ffigen helps with that but it's still a copy & paste job basically.

While we're at it I assume you don't have any objection to me adding in lstat at the same time?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/noojee/dart_posix/issues/2#issuecomment-968193502, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAG32OBGIXQSDWRR2NNPPSLUL4NJZANCNFSM5HKGTJWQ . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

ab36245 commented 2 years ago

I've finally managed to create a PR with fixes for MacOS stat. It's quite a bit of a rework so I hope it makes sense.

BTW, the PR forgets to mention that the is* getters on the Mode (which prompted this whole thing) now work correctly.

ab36245 commented 2 years ago

This is fixed with the release of 2.2.3