Re: [PATCH] AmigaOS 4 port contribution

["Zack Weinberg" <zack at codesourcery dot com>]

Marcus Comstedt <marcus@mc.pp.se> writes:

> "Zack Weinberg" <zack@codesourcery.com> writes:
>
>> I was afraid of that.  In that case we're going to need you to write
>> code which accepts an argument vector and carefully translates it to
>> a string, quoting each entry as necessary so that arbitrary characters
>> (other than NUL) are correctly preserved all the way to the second
>> argument to main() in the child process.  See pex-win32.c in the patch
>> I referred you to, for an example.
>
> Such code is in pex-amigaos.c in my patch.

And duplicated in collect2.c, which is another reason to make
pexecute() capable of handling what collect2 needs to do.

It is difficult for me to evaluate the correctness of this code
without any AmigaOS documentation.  Just on inspection, though, I
suspect that arguments that contain a splat character (*) will be
mangled (need to become **, as " becomes *"?), and an empty string
argument will vanish (probably should be translated to ""). I am also
nervous about arguments that contain ASCII-unprintable characters
(think filenames in extended character sets) and arguments that
contain both " and whitespace (is it still correct to surround the
whole with double quotes and put a splat in front of the embedded "?)

The whole is then fed to system(), which invokes a command line
interpreter (per the C standard).  I find it difficult to believe that
the CLI has no metacharacters other than *, ", and whitespace.  All
such would need to be escaped, or else you need to use a lower-level
interface that does not invoke the CLI.

A partial test of this interface could be accomplished by calling
pexecute() with an argument vector with 256 entries, each a single
byte, giving all possible byte values. In the child, verify that every
last one came through correctly.  This would not catch situations
where the shell silently passes through metacharacters out of their
preferred context.

By the way, don't use <ctype.h> macros anywhere in GCC; use
safe-ctype.h macros instead.  Also, you have a lot of code that looks
like

  (*s++) = '\"';

these parentheses are unnecessary.

I'm going to drop the OS-design discussion - this isn't the time or
place, and anyway, as you say neither C nor AmigaOS is likely to
change.  However, I hope you see from the concerns above why I have a
problem with this kind of API.

>> We would rather not have transitional states like that.  Besides, if
>> you do a full pexecute implementation now, the changes required in the
>> gcc directory will be smaller.
>
> Yes, but there were many other change requests to the patch to
> consider as well, and time is running short. 

You are not going to get this patch in for 3.4; slow down, take the
time to do it right, and it should be fine for 3.5.

>> It was felt that the API promised what it could not necessarily
>> deliver (namely pipes).  I think this is mistaken, but I'd be fine
>> with changing it to make the situation clearer.
>
> Hm.  Well, collect2 only needs a pipe as the output of the process,
> and in this case there should be no problem to fake it with a file if
> necessary.  collect_execute already has code to send output of the
> command to a file, so that much should be possible to deliver at
> least.

That is more or less what I said, but DJ did not agree.

zw