Re: Porting to OS/400: 4th patch release
Date: Wed, 22 Aug 2007 16:29:40 -0700
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
I didn't notice the header modifications. It sounds like that covers this
> > 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
> 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!
-- http://www.MoveAnnouncer.com The web change of address service Let webmasters know that your web site has movedReceived on 2007-08-23