Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

os400: additional support for options metadata #6574

Closed
wants to merge 1 commit into from

Conversation

@monnerat
Copy link
Contributor

@monnerat monnerat commented Feb 6, 2021

This is a blind attempt to support recent changes on OS/400.

  • Options metadata access using EBCDIC
  • Info data version 9 in EBCDIC
  • Sync ILE/RPG binding definitions with latest curl.h changes

A review and tests from @jonrumsey would be more than valuable.

Copy link
Contributor

@jonrumsey jonrumsey left a comment

Thanks for making these updates, they look good to me and can approve as-is. The only question I would have is about freeing the pointer returned by curl_easy_option_by_name_ccsid(), presumably this is just a regular heap free().

I think it may be nicer to have a 'free' routine (curl_easy_option_free), like there is for curl_slist to ensure that the storage is freed using the correct runtime/same used in malloc. I realise this wouldn't be required on other platforms where the pointer returned by curl_easy_option_by_name() is to a static string so maybe a free routine could no-op there.

Loading

@monnerat
Copy link
Contributor Author

@monnerat monnerat commented Feb 8, 2021

Many thanks for review.

... freeing the pointer returned by curl_easy_option_by_name_ccsid(...

No, this one should not be freed: The pointer returned by curl_easy_option_get_name_ccsid() should be!

... presumably this is just a regular heap free().

Yes, it is and should be freed with the free() C procedure, as it is already the case for strings returned by curl_easy_getinfo_ccsid().

I think it may be nicer to have a 'free' routine (curl_easy_option_free),

If you pass a curl_easyoption to such a procedure, it won't be able to free the (unattached) converted string. If you pass the allocated string, the function naming is some kind of inappropriate.

IMHO, the C interface is OK because we can easily include the stdlib.h header file to define free(). ILE/RPG is the real problem and maybe we should define an alias prototype for the free() procedure in curl.inc, Unlike for curl_slist_free_all(), the argument points to an atomic (i.e.: non-struictured) allocated block and the function can be used for non easy options related stuff, thus its name should be very generic. I would however avoid using the name curl_free() because of possible future name clash with the non OS400 code, What about curl_memory_free() ?

Loading

@jonrumsey
Copy link
Contributor

@jonrumsey jonrumsey commented Feb 8, 2021

Sorry, your are right, the pointer from curl_easy_option_get_name_ccsid() is the one to free.

LIBCURL *SRVPGM uses *CALLER activation group so an application should be using the same heap as the service program. I was concerned that the caller might be using a different storage model to the LIBCURL *SRVPGM (teraspace vs single level) so without performing the free() inside the library, an application could attempt to free the pointer from a different heap. Whilst I agree it is not essential, having a free routine inside the library provides symmetry.

curl_memory_free() seems like a reasonable name.

Loading

@monnerat
Copy link
Contributor Author

@monnerat monnerat commented Feb 8, 2021

I was concerned that the caller might be using a different storage model

You're right: this cannot be a simple alias since the effective free() procedure name changes when compiling in teraspace (although I've never try it!).

I can then implement curl_memory_free() as a simple wrapper.

Loading

@monnerat
Copy link
Contributor Author

@monnerat monnerat commented Feb 8, 2021

Commit 6faa8ee implements curl_memory_free().

In any case, the ILE/RPG binding will not work with a teraspace version of libcurl. I don't know how RPG evolved during the last 4 years, but at this time there was no support for teraspace pointers in it, you had to cheat with 20u 0 and cannot dereference them directly :-( The ILE/RPG binding is not ready for that, although this would be possible by defining a generic pointer type according to teraspace conditional and only defining other pointers with like().

Loading

@monnerat monnerat force-pushed the os400 branch 2 times, most recently from bb1885a to b0fb16a Feb 16, 2021
@monnerat monnerat force-pushed the os400 branch 2 times, most recently from afd6700 to 2ed25b6 Feb 24, 2021
@monnerat
Copy link
Contributor Author

@monnerat monnerat commented Feb 24, 2021

@jonrumsey : I removed the curl_memory_free() from this PR, as curl_free() is already the same procedure. I updated the documentation accordingly.

Loading

@monnerat monnerat force-pushed the os400 branch 3 times, most recently from 930eebf to 85100a3 Apr 15, 2021
New functions curl_easy_option_by_name_ccsid() and
curl_easy_option_get_name_ccsid() allows accessing metadata in alternate
character encoding.

This commit also updates curl_version_info_ccsid() to handle info version 9
and adds recent definitions to the ILE/RPG include file.

Documentation updated accordingly.
@bagder
Copy link
Member

@bagder bagder commented Apr 22, 2021

Thanks!

Loading

@monnerat monnerat deleted the os400 branch Apr 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants