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

Jump to bottom

os400: additional support for options metadata #6574

Closed
wants to merge 1 commit into from
Closed

os400: additional support for options metadata #6574

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.

jonrumsey
jonrumsey approved these changes Feb 8, 2021
View reviewed changes
Copy link
Contributor

@jonrumsey jonrumsey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@monnerat
Copy link
Contributor Author

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

@jonrumsey
Copy link
Contributor

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.

@monnerat
Copy link
Contributor Author

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.

@monnerat
Copy link
Contributor Author

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

@monnerat
Copy link
Contributor Author

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

@monnerat monnerat force-pushed the os400 branch 3 times, most recently from 930eebf to 85100a3 Compare April 15, 2021 15:08
@monnerat
os400: additional support for options metadata
789976b 
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 bagder closed this in a2a8f9d Apr 22, 2021
@bagder
Copy link
Member

bagder commented Apr 22, 2021

Thanks!

@monnerat monnerat deleted the os400 branch April 22, 2021 07:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jonrumsey jonrumsey jonrumsey approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@monnerat @jonrumsey @bagder