Add fgetattrlist() #93

macportsraf · 2024-07-15T00:07:34Z

This adds an implementation of fgetattrlist(). Tested on 10.14.6, 10.6.8, and 10.4.11 (ppc) but only the 10.4 test means anything. The test is very basic. It just calls the function on the current directory's file descriptor, and only requires that it doesn't return an error.

I noticed a strange thing when testing on 10.4 (ppc). The first time I run:

PATH=/opt/local/bin:$PATH CC=gcc-apple-4.2 CFLAGS=-isystem`pwd`/tiger_only/include make test

I get an error:

ld: file not found: src/fdopendir.dl.o.inode32.ppc

But if I run it again, it works. This happens with or without this change. I suspect that it doesn't matter.

fhgwright · 2024-07-16T00:47:31Z

A few notes:

I don't think it's necessary to have separate feature flags for fgetattrlist() and fsetattrlist(), given that they appeared at the same time and are documented as going hand-in hand. It may or may not make sense in the spirit of egalitarianism to rename the feature to FXETATTRLIST. :-) A similar issue might apply to the sources.
PR Mostly SDK-related changes, including __DARWIN_C_LEVEL stuff #92 makes substantial changes that interact with this, so you should look at that, or wait until it's merged to finalize this one. Though sharing the feature flag(s) from fsetattrlist() without a rename might not conflict.
This might be a good opportunity to add a test for fsetattrlist(), which was previously neglected.
There's a subtle potential problem in that these functions aren't supported by all filesystems. It shouldn't be an issue for HFS+ or APFS, but if someone tried to run the test on a weird filesystem it might fail. I don't think it's worth treating that as a pass, but it probably makes sense to provide a more informative error in that case. I think all that's needed is to try a getattrlist() and check the error code. I expect that the failure can be seen by using a FAT32 partition on a USB stick.

fhgwright · 2024-07-16T00:53:40Z

I noticed a strange thing when testing on 10.4 (ppc). The first time I run:
PATH=/opt/local/bin:$PATH CC=gcc-apple-4.2 CFLAGS=-isystem`pwd`/tiger_only/include make test
I get an error:
ld: file not found: src/fdopendir.dl.o.inode32.ppc
But if I run it again, it works. This happens with or without this change. I suspect that it doesn't matter.

That sounds like a missing dependency in the Makefile, though I haven't seen it myself.

BTW, if you're working from the latest master and not the release version, then tiger-only/include no longer exists, and no special setup is needed for 10.4 (other than the compiler choice).

fhgwright · 2024-07-16T01:01:30Z

A couple of other notes:

I think it's best to put implementations and tests into separate commits, to facilitate reproducing the test failure when the implementation is missing. The implementation should come first, to avoid creating a "pothole" in the history.
This should probably be a draft until Mostly SDK-related changes, including __DARWIN_C_LEVEL stuff #92 is merged, and this one is adjusted as needed.

macportsraf · 2024-07-17T02:56:37Z

A few notes:

1. I don't think it's necessary to have separate feature flags for `fgetattrlist()` and `fsetattrlist()`, given that they appeared at the same time and are documented as going hand-in hand.  It may or may not make sense in the spirit of egalitarianism to rename the feature to `FXETATTRLIST`. :-) A similar issue might apply to the sources.

I've removed __MP_LEGACY_SUPPORT_FGETATTRLIST__.

2. PR [Mostly SDK-related changes, including __DARWIN_C_LEVEL stuff #92](https://github.com/macports/macports-legacy-support/pull/92) makes substantial changes that interact with this, so you should look at that, or wait until it's merged to finalize this one.  Though sharing the feature flag(s) from `fsetattrlist()` _without_ a rename might not conflict.

I'm happy to let others decide that.

3. This might be a good opportunity to add a test for `fsetattrlist()`, which was previously neglected.

Added. The test needed fgetattrlist() (taken from the first code sample in the manual entry), so maybe the other fgetattrlist() (taken from its manual entry) is no longer necessary, but it's harmless.

4. There's a subtle potential problem in that these functions aren't supported by all filesystems.  It shouldn't be an issue for `HFS+` or `APFS`, but if someone tried to run the test on a weird filesystem it might fail.  I don't think it's worth treating that as a pass, but it probably makes sense to provide a more informative error in that case.  I think all that's needed is to try a `getattrlist()` and check the error code.  I expect that the failure can be seen by using a FAT32 partition on a USB stick.

I've added some output in the case of test failure to state that it might be correctly failing due to an unexpected filesystem type.

macportsraf · 2024-07-17T03:00:07Z

That sounds like a missing dependency in the Makefile, though I haven't seen it myself.

It was just a time error on the laptop. For some reason, it doesn't set the time until I go to the system preferences and look at the time settings.

BTW, if you're working from the latest master and not the release version, then tiger-only/include no longer exists, and no special setup is needed for 10.4 (other than the compiler choice).

Thanks.

macportsraf · 2024-07-17T03:04:11Z

A couple of other notes:

1. I think it's best to put implementations and tests into separate commits, to facilitate reproducing the test failure when the implementation is missing.  The implementation should come first, to avoid creating a "pothole" in the history.

Done.

2. This should probably be a draft until [Mostly SDK-related changes, including __DARWIN_C_LEVEL stuff #92](https://github.com/macports/macports-legacy-support/pull/92) is merged, and this one is adjusted as needed.

It'll be here when you want it.

fhgwright · 2024-07-17T03:45:05Z

include/MacportsLegacySupport.h

@@ -186,7 +186,7 @@
 /*  realpath() on < 1060 does not support modern NULL buffer usage */
 #define __MP_LEGACY_SUPPORT_REALPATH_WRAP__   (__MPLS_TARGET_OSVER < 1060)

-/* setattrlistat */
+/* fsetattrlistat */


Should mention both functions now.

fhgwright · 2024-07-17T03:45:14Z

src/fgetattrlist.c

+
+#if __MPLS_TARGET_OSVER < 1080
+    /*
+     * Older systems don't correctly check if no attributes are to be set, which usually


Copy-'n-paste error: "set" isn't really correct here. Maybe "read" or "obtained".

Based on the description, I don't know if the problem addressed here exists on reads at all, though as you say, the "fix" probably doesn't hurt.

fhgwright · 2024-07-17T03:57:11Z

Note that since this interacts with __DARWIN_C_LEVEL, it will also need the obvious one-line addition to manual_tests/darwin_c.c, which won't appear until #92 is merged. This is mentioned in the comment in MacportsLegacySupport.h as (to be) updated. "Need" is in the sense of "for completeness of the test", not "for correct functioning".

That's the trouble with adding features while the framework is in flux. :-)

README.md will need an update as well.

macportsraf · 2024-07-18T01:19:44Z

Note that since this interacts with __DARWIN_C_LEVEL, it will also need the obvious one-line addition to manual_tests/darwin_c.c, which won't appear until #92 is merged. This is mentioned in the comment in MacportsLegacySupport.h as (to be) updated. "Need" is in the sense of "for completeness of the test", not "for correct functioning".

That's the trouble with adding features while the framework is in flux. :-)

Of course, this patch could go in first, and the other patch could be rebased to take it into account. But I assume there must be good reasons not to do that. And there's no urgency for this.

And, presmably, I'll need to change __MP_LEGACY_SUPPORT_FSETATTRLIST__ to __MP_LIB_SUPPORT_FSETATTRLIST__ in src/fgetattrlist.c when the time comes.

README.md will need an update as well.

Done. fsetattrlist wasn't in there either but it is now.

fhgwright · 2024-07-18T01:31:17Z

@macportsraf

Of course, this patch could go in first, and the other patch could be rebased to take it into account. But I assume there must be good reasons not to do that. I don't understand why this patch would have anything to do with manual tests, since the tests aren't manual, but that's OK. I probably don't need to understand. Just let me know what change to make to manual_tests/darwin_c.c when the time comes.

Well, #92 came first and is much more extensive. And once you see it, you'll see what the obvious addition is for fgetattrlist().

The darwin_c tests are only manual for a weird reason involving the Makefile.

README.md will need an update as well.

Done. fsetattrlist wasn't in there either but it is now.

Ah, you're right. I'd missed that. I'd also missed the header typo until after #92. :-)

fhgwright · 2024-07-21T20:51:22Z

include/MacportsLegacySupport.h

@@ -186,7 +186,7 @@
 /*  realpath() on < 1060 does not support modern NULL buffer usage */
 #define __MP_LEGACY_SUPPORT_REALPATH_WRAP__   (__MPLS_TARGET_OSVER < 1060)

-/* setattrlistat */
+/* fsetattrlistat, fgetattrlistat */


There's a trivial lexical conflict here with the updated master, due to non-conflicting changes on adjacent lines.

fhgwright · 2024-07-21T20:51:26Z

src/fgetattrlist.c

+
+/* MP support header */
+#include "MacportsLegacySupport.h"
+#if __MP_LEGACY_SUPPORT_FSETATTRLIST__


The flag name needs to change to __MPLS_LIB_SUPPORT_FSETATTRLIST__, to match the new naming scheme.

Also, I think it's visually better to put a blank line between the include and the conditional. I already did that in fsetattrlist.c, but it wouldn't have been in master at the time you created this PR.

fhgwright · 2024-07-21T20:51:31Z

src/fgetattrlist.c

+    return ret;
+}
+
+#endif  /* __MP_LEGACY_SUPPORT_FSETATTRLIST__ */


Same name change here.

fhgwright · 2024-07-21T20:54:19Z

@macportsraf
With the above changes to the feature commit, and the previously mentioned one-line addition to the test commit, this will be properly referenced to the new master. I already tested a local version with those changes.

macportsraf · 2024-07-22T11:11:56Z

Thanks. It's updated.

fhgwright · 2024-07-22T19:40:08Z

@macportsraf

Thanks. It's updated.

Almost. :-)

As previously mentioned, the test commit should include this:

--- a/manual_tests/darwin_c.c
+++ b/manual_tests/darwin_c.c
@@ -212,6 +212,7 @@ int timespec_get = 0;
 #ifdef _SC_PHYS_PAGES
 #error _SC_PHYS_PAGES is unexpectedly defined
 #endif
+int fgetattrlist = 0;
 int fsetattrlist = 0;
 #endif /* __DARWIN_C_LEVEL < __DARWIN_C_FULL */

macportsraf · 2024-07-23T03:37:16Z

Thanks. It's there now.

fhgwright · 2024-07-23T19:33:14Z

@macportsraf

Thanks. It's there now.

Umm, you didn't actually change it:

MacPro:macports-legacy-support fw$ git diff 5d60b93 b34b0bb
MacPro:macports-legacy-support fw$

Did you forget a -a perhaps?

macportsraf · 2024-07-24T00:52:18Z

Oops. It's there now.

fhgwright · 2024-07-24T02:44:02Z

@macportsraf

Oops. It's there now.

Yup.

That now matches what I'd already tested, on:

Mac OS X 10.4.11 8S165, PPC, Xcode 2.5 8M2558
Mac OS X 10.4.11 8S2167, i386, Xcode 2.5 8M2558
Mac OS X 10.5.8 9L31a, PPC, Xcode 3.1.4 9M2809
Mac OS X 10.5.8 9L31a, i386, Xcode 3.1.4 9M2809
Mac OS X 10.5.8 9L31a, x86_64, Xcode 3.1.4 9M2809
Mac OS X 10.5.8 9L31a, PPC (i386 Rosetta), Xcode 3.1.4 9M2809
Mac OS X 10.6.8 10K549, i386, Xcode 3.2.6 10M2518
Mac OS X 10.6.8 10K549, x86_64, Xcode 3.2.6 10M2518
Mac OS X 10.6.8 10K549, PPC (i386 Rosetta), Xcode 3.2.6 10M2518
Mac OS X 10.7.5 11G63, x86_64, Xcode 4.6.3 4H1503
OS X 10.8.5 12F2560, x86_64, Xcode 5.1.1 5B1008
OS X 10.9.5 13F1911, x86_64, Xcode 6.2 6C131e
OS X 10.10.5 14F2511, x86_64, Xcode 7.2 7C68
OS X 10.11.6 15G22010, x86_64, Xcode 8.1 8B62
macOS 10.12.6 16G2136, x86_64, Xcode 9.2 9C40b
macOS 10.13.6 17G14042, x86_64, Xcode 10.1 10B61
macOS 10.14.6 18G9323, x86_64, Xcode 11.3.1 11C505
macOS 10.15.7 19H15, x86_64, Xcode 12.4 12D4e
macOS 11.7.10 20G1427, x86_64, Xcode 13.2.1 13C100
macOS 11.7.10 20G1427, arm64, Xcode 13.2.1 13C100
macOS 12.7.5 21H1222, x86_64, Xcode 14.2 14C18
macOS 12.7.5 21H1222, arm64, Xcode 14.2 14C18
macOS 13.6.7 22G720, arm64, Xcode 15.2 15C500b
macOS 14.5 23F79, arm64, Xcode 15.4 15F31d

That actually understates it, since I also built and ran the tests with all compatible SDKs not earlier than the target OS.

@mascguy
LGTM

mascguy · 2024-07-24T02:46:30Z

@mascguy
LGTM

Great, I'll go ahead and merge

macportsraf mentioned this pull request Jul 15, 2024

ruby-devel: update to upstream, fix build on < 10.7 macports/macports-ports#24868

Closed

12 tasks

macportsraf force-pushed the fgetattrlist branch 2 times, most recently from da37455 to 46c3af0 Compare July 15, 2024 01:07

macportsraf force-pushed the fgetattrlist branch 2 times, most recently from 86974c4 to c581862 Compare July 17, 2024 02:47

fhgwright reviewed Jul 17, 2024

View reviewed changes

macportsraf force-pushed the fgetattrlist branch from c581862 to 8cc1c7e Compare July 18, 2024 00:44

fhgwright mentioned this pull request Jul 20, 2024

Mostly SDK-related changes, including __DARWIN_C_LEVEL stuff #92

Merged

fhgwright reviewed Jul 21, 2024

View reviewed changes

src/fgetattrlist.c Outdated

return ret;

}

#endif /* __MP_LEGACY_SUPPORT_FSETATTRLIST__ */

Copy link

Contributor

fhgwright Jul 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same name change here.

fhgwright mentioned this pull request Jul 21, 2024

Add two new queue macros with tests, plus a warning fix. #94

Merged

Add fgetattrlist()

70cc77f

macportsraf force-pushed the fgetattrlist branch from 8cc1c7e to 5d60b93 Compare July 22, 2024 11:03

macportsraf force-pushed the fgetattrlist branch from 5d60b93 to b34b0bb Compare July 23, 2024 03:36

Add tests for fgetattrlist() and fsetattrlist()

f6d11ae

macportsraf force-pushed the fgetattrlist branch from b34b0bb to f6d11ae Compare July 24, 2024 00:51

mascguy merged commit b9309fc into macports:master Jul 24, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add fgetattrlist() #93

Add fgetattrlist() #93

macportsraf commented Jul 15, 2024 •

edited

Loading

fhgwright commented Jul 16, 2024

fhgwright commented Jul 16, 2024

fhgwright commented Jul 16, 2024

macportsraf commented Jul 17, 2024

macportsraf commented Jul 17, 2024

macportsraf commented Jul 17, 2024

fhgwright Jul 17, 2024

macportsraf Jul 18, 2024

fhgwright Jul 17, 2024

macportsraf Jul 18, 2024

fhgwright commented Jul 17, 2024

macportsraf commented Jul 18, 2024 •

edited

Loading

fhgwright commented Jul 18, 2024

fhgwright Jul 21, 2024

fhgwright Jul 21, 2024

fhgwright Jul 21, 2024

fhgwright commented Jul 21, 2024

macportsraf commented Jul 22, 2024

fhgwright commented Jul 22, 2024

macportsraf commented Jul 23, 2024

fhgwright commented Jul 23, 2024

macportsraf commented Jul 24, 2024

fhgwright commented Jul 24, 2024

mascguy commented Jul 24, 2024

Add fgetattrlist() #93

Add fgetattrlist() #93

Conversation

macportsraf commented Jul 15, 2024 • edited Loading

fhgwright commented Jul 16, 2024

fhgwright commented Jul 16, 2024

fhgwright commented Jul 16, 2024

macportsraf commented Jul 17, 2024

macportsraf commented Jul 17, 2024

macportsraf commented Jul 17, 2024

fhgwright Jul 17, 2024

Choose a reason for hiding this comment

macportsraf Jul 18, 2024

Choose a reason for hiding this comment

fhgwright Jul 17, 2024

Choose a reason for hiding this comment

macportsraf Jul 18, 2024

Choose a reason for hiding this comment

fhgwright commented Jul 17, 2024

macportsraf commented Jul 18, 2024 • edited Loading

fhgwright commented Jul 18, 2024

fhgwright Jul 21, 2024

Choose a reason for hiding this comment

fhgwright Jul 21, 2024

Choose a reason for hiding this comment

fhgwright Jul 21, 2024

Choose a reason for hiding this comment

fhgwright commented Jul 21, 2024

macportsraf commented Jul 22, 2024

fhgwright commented Jul 22, 2024

macportsraf commented Jul 23, 2024

fhgwright commented Jul 23, 2024

macportsraf commented Jul 24, 2024

fhgwright commented Jul 24, 2024

mascguy commented Jul 24, 2024

macportsraf commented Jul 15, 2024 •

edited

Loading

macportsraf commented Jul 18, 2024 •

edited

Loading