Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

NOIRLAB: several patches for libos.a #346

Merged
merged 8 commits into from Jan 29, 2024
Merged

Conversation

olebole
Copy link
Member

@olebole olebole commented Jan 9, 2024

There is a number of patches that fix several glitches in libos.a:

a6459af - increased 'fname' buffer size to avoid overflow
48b2f61 - fixed fcancel() macro for OSX
997ff1c - deprecated use of rcmd() on OSX systems
28a9b9e - ensure null termination following strncpy(), gcc 8 -Wstringop-truncation err
1dfb2a7 - ensure null termination following strncpy(), gcc 8 -Wstringop-truncation err
ab2ec08 - ensure null termination following strncpy(), gcc 8 -Wstringop-truncation err
a55a788 - fix sign compare
0e72cfc - fix clobber warning from linux64
d8226de - fix compiler warnings from linux64

The commit ab2ec08 looks a bit weird:

diff --git a/unix/os/zfiond.c b/unix/os/zfiond.c
index 294a6eb2b..33d779b9c 100644
--- a/unix/os/zfiond.c
+++ b/unix/os/zfiond.c
@@ -375,6 +375,11 @@ ZOPNND (
                sockaddr.sun_family = AF_UNIX;
                strncpy (sockaddr.sun_path,
                    np->path1, sizeof(sockaddr.sun_path));
+#ifdef MACOSX
+               sockaddr.sun_path[103] = '\0';  // ensure NULL termination
+#else
+               sockaddr.sun_path[107] = '\0';  // ensure NULL termination
+#endif
 
                /* Connect to server. */
                if (fd >= MAXOFILES || (connect (fd,
[… same change few lines later …]

The commit currently in the PR has MACOSX replaced with __APPLE__ (because we consequently use compiler provided flags instead of self-defined if possible). It is however unclear where the magic numbers 103 and 107 come from and why they differ for macOS. I will leave it out yet until we understand it (maybe @mjfitzpatrick may explain).

The other suspicious commit is 48b2f61, which defines fcancel() as setvbuf() on macOS

iraf/unix/os/zxwhen.c

Lines 90 to 96 in 3761a03

#ifdef MACOSX
#define _IONBF 2 /* No buffering. */
#define fcancel(fp) setvbuf(fp, NULL, _IONBF, 0);
#else
//#define fcancel(fp) ((fp)->_r = (fp)->_w = 0)
#define fcancel(fp)
#endif

First, setvbuf() is not macOS specific but part of the standard C library (and therefore also available for Linux). This is changed by the commit in this PR. Then, hard-coding _IONBF instead of using the proper include file (stdio.h) is bad style, also adjusted here. And finally, setvbuf() is not what fcancel() is supposed to do, namely to cancel any buffered output. Instead, it just makes the output unbuffered. i.e. when applying this patch, stdout will become unbuffered for the rest of the session. Maybe @mjfitzpatrick can comment on that, otherwise I tend to remove this commit from the PR as well.
Probably the better solution (although non-portable) is to use fpurge() on macOS (and BSD variants), and __fpurge() on Linux (and glibc variants, like HURD). This is implemented in #366.

Then there is 997ff1c, which basically disables network computing capabilities on macOS

iraf/unix/os/zfioks.c

Lines 2048 to 2057 in 997ff1c

static int
rcmd_(char **ahost, int inport, const char *locuser, const char *remuser,
const char *cmd, int *fd2p)
{
int res = 0;
#ifndef MACOSX
res = rcmd (ahost, inport, locuser, remuser, cmd, fd2p)
#endif
return res;
}

I am not sure whether network computing capabilities are still needed on Linux; at least standard remove shells (rsh) are deprecated everywhere, not just on macOS. If not, we could think about removing the networking layer completely, significantly simplifying the code in unix/os. @mjfitzpatrick?

@olebole olebole marked this pull request as draft January 9, 2024 09:44
@olebole olebole changed the title NOIRLAB: several patches for unix/os NOIRLAB: several patches for libos.a Jan 14, 2024
@olebole olebole marked this pull request as ready for review January 27, 2024 16:50
@olebole olebole marked this pull request as draft January 29, 2024 12:21
@olebole olebole marked this pull request as ready for review January 29, 2024 14:09
@olebole olebole merged commit de4e326 into iraf-community:main Jan 29, 2024
3 checks passed
@olebole olebole deleted the unix-os branch January 29, 2024 14:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants