Re: wip/glusterfs to pkgsrc filesystems/glusterfs

[Jonathan Schleifer <js%NetBSD.org@localhost>]

Am 01.03.23 um 00:49 schrieb Alexander Schreiber:

> > Maybe ONLY_FOR_PLATFORM would be better here? This smells like the code
> > isn't written portably, so probably will fail on anything but amd64 and
> > aarch64 anyway?
>
> I've successfully tested (built & run) this on all 64bit platforms I have
> available for testing: aarch64, amd64, sparc64. It cannot run (but can be
> built) on any 32bit platform, since the code tries to atomically update
> a 64bit value eventually (userspace-rcu), which is not something that is
> going to work on 32bit (unless you got a weird one). New 32bit platforms
> are unlikely to spring up, but pkgsrc help already mentions e.g. riscv64.
>
> I would prefer a cleaner ONLY_FOR_64BIT (or similar), but that doesn't
> appear to exist.
>
> There were two portability issues that broke the build on !Linux, but the
> patches address that. And in fact it was alignment-related breakage
> (SIGBUS) on sparc64 (fixed sometime between 9.3 and 10.3 upstream) that
> got me to work on this ;-)

In that case, ${LP32PLATFORMS} as Thomas recommended is the right way to 
go. I didn't expect it to work on anything but the big 64 bit platforms, 
nice to hear it works on SPARC64 :).

> > > -$NetBSD: patch-configure,v 1.5 2020/09/27 01:13:11 manu Exp $
> > > +$NetBSD$
> >
> > I know the current package already does this, but it's really bad practice.
> > Only patch configure.ac and then regenerate. Do not patch the generated
> > file.
>
> Just patching configure.ac and regenerating was my initial approach
> because it is obviously the right way to do. Except that ran into a
> whole pile of issues, which made me realize _why_ people do the ugly
> hack of patching ./configure.

Let's fix this properly. What issues did you run into?

> > patch-libglusterfs-src-glusterfs-dict.h is a bit sketchy. How about checking
> > for __LP64__ instead?
>
> Sorry, that is wrong and makes the same mistake as the original code
> of using machine word size as a (very bad) proxy for sizeof(time_t).
> In fact, NetBSD is the prime example why this is wrong: on a 32bit
> machine arch (i386), NetBSD still has sizeof(time_t) == 8. _Actually_
> having configure check for the value of sizeof(time_t) is the only
> correct approach here.

That's not what the description of the patch says:

> Do not take for granted that __WORDSIZE is defined to distinguish 
> between 32 and 64 bit platforms. Instead use ULONG_MAX from <limits.h>
> which is mandated by ISO C99.

__LP64__ would probably be the best fallback for that.

> Still not a good solution, as __WORDSIZE is used as a proxy for sizeof(time_t),
> but some superficial tests indicate that sizeof(time_t) seems to track with
> machine word size on reasonably current platforms.

AC_CHECK_SIZEOF(time_t) would be the correct solution for that, and not 
infer it, as you say yourself.

Now, looking at the actual patch, that seems to be what it does, so 
you'll need to fix the patch description.

--
Jonathan