Many thanks Dan, for your great review job on my lines and the
time you spent doing it. You have pointed several repetitive bugs that
would have lead in run-time problems sooner or later, and your advices
improve considerably the overall quality of the OS/400 code.

The reply was long to come, 'cause I was working full-time on your
suggestions.

There are a few of your remarks that I haven't applied and I tell you
here why:

> +#pragma enum(int)

I have not removed it as you suggest. As mentionned in the README.OS400
file, it is not possible to have a char or an int as varargs procedures
tagfield on OS/400 (It simply does not work!). Since enums are used for
this purpose, I have to force their type to int. Installed curl header
files are updated by the scripts accordingly. The statement is hardcoded
here, because curl library compilation includes the unmodified header
files.

> +curl_version_info_data *
> +curl_version_info_ccsid(CURLversion stamp, unsigned int ccsid)
> +
> +{
> + curl_version_info_data * p;

> You may want to put an assert in here on the
sizeof(curl_version_info_data) ...

Quite impossible to test on the size. However, I've put an assert on
CURLVERSION_NOW == CURLVERSION_FOURTH. I have also added a dynamic test
to reject callers that have been compiled with a newer version.

> +Curl_is_formadd_string(CURLformoption option)

> Probably a good idea to put an assert on CURLFORM_LASTENTRY ...

Yes, but how ? The C preprocessor has no idea about the relationship
between two enum members. That would have been possible if the
CURLOPT_xxx had been #defined. And I'm not in favor of a run-time check
+ fprintf(stderr, ...) because it does not concern the calling
application. As long as the calling application does not use the new
CURLFORM option(s), it should be transparent. Of course, use of new
options will be rejected, because unknown.

> +curl_formadd_ccsid(struct curl_httppost * * httppost,

> This function is > 240 lines long and could (should) be split into
several more for readability and easier maintenance. Long function
length is one aspect of curl coding style you don't need to follow :^)

I agree with your remark. I applied the single loop transform with
reallocation you suggest, making it more readable (I hope!). However it
is still about the same size, and I don't think splitting it will help
readability. In fact, this is indexed on the FormAdd() procedure
complexity: see below.

> + if (option == CURLFORM_CONTENTTYPE) {
> + if (Curl_formadd_convert(lforms, contentx, lengthx,
contentccsid)
> + < 0) {

> I don't see why CURLFORM_CONTENTTYPE is special cased like this. What
exactly is going on here?

By looking at the FormAdd() procedure code, it seems there is a way to
have several data sources for the same form. Since COPYCONTENTS and
CONTENTSLENGTH argument order is not defined, I have to defer
COPYCONTENTS conversion to a point I'm sure I know both of them if
specified, and it looks CONTENTTYPE options are such points (the other
is END). But I may be wrong: FormAdd() code is very hard to understand.
The same applies to COPYNAME/NAMELENGTH, but I admit there is at most a
single COPYNAME per curl_formadd_ccsid().

> Probably a good idea to put an assert on CURLOPT_LASTENTRY on entry to
this function to catch the case where a new option is added without
modifying this function accordingly.

Same preprocessor/enum remark as above.

> Some of the functions in this file look like they could be useful to
other ports and are have no dependencies on curl whatsoever (e.g.
> Curl_getnameinfo_a, Curl_getaddrinfo_a). Would it make sense to move
those into another source file so they could be more easily used by
other OS/400 software porters?

In my point of view, they are already in "another" source file ;-) The
code in os400sys.c is a small complement to the QADRT ascii wrapping
facility from IBM, that is far from being complete. I agree this code
can be useful to port other packages to OS/400, but I don't think curl
has the goal to provide such a wide OS/400 coding support... In any
case, this is opensource and anybody needing it can copy it.

> + if (size <= buf->size)
> + return buf->buf;

> This will be wasting space by returning too large a buffer. It might
be better to realloc the buffer in this case, too.

Only if byte gain is above a certain amount (MIN_BYTE_GAIN #defined as
1024) to avoid realloc() overhead.

Many thanks also for the warning report: I fixed unused variables,
signed/unsigned comparisons, etc. Absence of prototypes is normal,
because including the prototype file will also redefine system procedure
names: this is needed in all but os400.sys, and splitting the include
file into a prototype file and definition file does not add many
advantages (only usage/definition coherence checks).
I submitted the latest version to the same type of checks with the
OS/400 C compiler without significant reporting.

Please find the latest release in attachment. Unless you send me a red
flag, I'll commit it soon.
Many thanks again
Patrick