I prefer to do the review via email rather than in the Savannah bug
tracker which has pretty annoying markup.

I would appreciate a somewhat comprehensive commit message or ChangeLog
for this set of patches, at least explaining some of the less obvious

> +set -x
> +if [ ! ${PLATFORM} = "OS/390" ]; then
> +$CC $CFLAGS $LDFLAGS -L"$OUTLIB" -o "$OUTDIR/makenew$EXEEXT" $objs -
> +else
> "$OUTDIR/makenew$EXEEXT"
> +fi

We don't want set -x here.

Is the point of this that the compiler on OS/390 doesn't allow the -o
option to come after the objects?  If so we should just change the
command line order on all systems; no need to check for platforms here.
Other compilers don't care about the order in which -o comes so it can
just come early for all of them.

> -# define __stat stat
> +# define __gnustat stat

I suppose OS/390 already defines __stat to something else?  All this
code in glob.c and fnmatch.c is not really owned by GNU make, we import
it from elsewhere.  But it looks like we'll have to do something about

> +#ifdef __MVS__
> +int execvpe(const char* name,
> +                       char* const argv[],
> +                       char* const envp[]) {

Please follow the GNU coding style here and in all code.  That means
the return type on its own line, braces on their own lines (both
function and inner block braces), one space between a function name and
the open paren, etc.  You can find info here:


Also, I'm not sure why we need this function.  Can you provide some
comments or similar here explaining why it's needed and what it does?

> +int __setccsid(int fd, int ccsid) 
> +{
> +  attrib_t attr;
> +  int rc;
> +

As above please follow the coding standards.  Also some comments here
are needed to explain what this is doing.  I also wonder if it wouldn't
be better to collect all this z/OS support into a separate C file
rather than sprinkling it throughout the rest of the code (I realize
that's the model that was used in the past but it's not a good one).

Finally, I don't think it's a good idea to use identifiers that start
with two underscores like this (__setccsid) because these are reserved
for use by the compiler, according to the C standard.  Instead we
should choose a name that won't conflict, unless there's some reason
that these functions must have this specific name (like you're
overriding a system function or something).

> +#ifdef __MVS__
> +#include "os390.h"
> +# include <fcntl.h>
> +# define pipe(_p)        __pipe_ascii(_p)
> +#endif

It seems like it shouldn't be needed to include "os390.h" here. 
Especially if the custom parts of z/OS are restricted to a new file,
this header can just be included there I expect.

> +#ifndef __MVS__
> +      //TODO: Implement alternative
>        void *sem = acquire_semaphore ();
> +#endif

I don't think this is the right way to handle this.  If the port
doesn't support output sync, then you should ensure that NO_OUTPUT_SYNC
is defined for this port, then none of this code will be compiled.

>  /* These track the state of the jobserver pipe.  Passed to child
> instances.  */
> -static int job_fds[2] = { -1, -1 };
> +static int job_fds[2] = {};

I'm pretty sure this will break other ports.  Why is it needed?

> +    if ($osname eq "") {
> +      eval "chop (\$osname = `uname -nmsr 2>&1`)";

By ignoring the comment here about /bin/sh -c it seems you'll cause
issues with other ports.  Can you explain why this is needed?  If
/bin/sh -c doesn't work on zOS maybe we need to find a better way to
handle this.  Also I don't understand why you're retesting $osname
directly after it was just tested.

In general I'm always happy to receive ports to new platforms but I
will remind that I can't test these going forward.  Hopefully someone
on the IBM team will commit to subscribing to the bug-make@gnu.org list
and signing up to test pre-releases when they come out, to make sure
that the zOS support is still working.

Thanks for submitting this, and I hope the review was useful!

Reply via email to