On Wed, Aug 22, 2007 at 05:46:54PM +0200, Patrick Monnerat wrote:
> 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.

I didn't notice the header modifications. It sounds like that covers this
potential problem.

> > 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.

I was thinking of a run-time assert, but you're right--it will be rejected
anyway so an assert would be rudundant.

> > 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().

Ah, I see. Maybe the curl API documentation should be changed to force
users to set CONTENTSLENGTH before COPYCONTENTS, or, at least, make this
a restriction of the OS/400 port. It would be nice to have a comment in
the code explaining this, too.

> > 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.

I don't agree here--the default behaviour in this function
(curl_easy_setopt_ccsid) is not to reject an unknown option like the last
case, but to do the option without any string conversion. If
an app writer comes to depend on this behaviour of a new option, then this
function is later fixed, it will break the app. Some kind of check for
this ought to be in there, either an assert or returning CURLE_FAILED_INIT
or something. That kind of unconditional failure will be immediately
apparent to the library packager or user until fixed, and, IMHO, is
preferable to silently failing to run properly.

> > 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.

I put the thread buffer code and anything that depended on it into a curl-
specific category, whereas the rest seemed pretty generic. It's a minor
point.

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

One other thing would be good to add: appropriate #ifdef USE_QSOSSL,
CURL_DISABLE_LDAP, and HAVE_GSSAPI blocks around the relevant sections
in os400sys.c to allow compiling those options out if desired.

I also spotted a couple more places where ICONV_MAX_EXPANSION should be
used: in Curl_SSL_Strerror_a and Curl_ldap_err2string_a.

Otherwise, it looks good to me!

>>> Dan

-- 
http://www.MoveAnnouncer.com              The web change of address service
          Let webmasters know that your web site has moved