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

zos: successfully build c-ares codebase on z/OS systems #390

Merged
merged 15 commits into from Dec 24, 2020

Conversation

dhrumilrana
Copy link
Contributor

This PR is for building c-ares on z/OS systems. The PR makes changes couple of header files to introduce MVS checks for z/OS specific code.

DHRUMIL and others added 13 commits December 4, 2020 16:12
* Change to build c-ares on z/OS.

* Change to build unit tests.

* Fixed formatting, removed MVS check from ares_setup.h and added a check to ares.h.

* Change to build c-ares on z/OS.

* Change to build unit tests.

* Fixed formatting, removed MVS check from ares_setup.h and added a check to ares.h.

* adding the remaining z/OS changes.

* Made minor changes to follow code guidelines and optimize indexing.

* Minor fixes to code.

* Minor code style fix.

* Move MVS specifc includes to top.

* Add MVS check to include a header file.

Co-authored-by: DHRUMIL <DHRUMIL@ZOSCAN2B.torolab.ibm.com>
* Change to build c-ares on z/OS.

* Change to build unit tests.

* Fixed formatting, removed MVS check from ares_setup.h and added a check to ares.h.

* Change to build c-ares on z/OS.

* Change to build unit tests.

* Fixed formatting, removed MVS check from ares_setup.h and added a check to ares.h.

* adding the remaining z/OS changes.

* Made minor changes to follow code guidelines and optimize indexing.

* Minor fixes to code.

* Minor code style fix.

* Move MVS specifc includes to top.

* Add MVS check to include a header file.

Co-authored-by: DHRUMIL <DHRUMIL@ZOSCAN2B.torolab.ibm.com>
@coveralls
Copy link

coveralls commented Dec 13, 2020

Coverage Status

Coverage increased (+0.002%) to 88.596% when pulling 99d84d5 on ibmruntimes:ibm-zos into 5ff0f6a on c-ares:master.

@bradh352
Copy link
Member

looks pretty good. Just need a couple of fixups.

  1. Remove the assert
  2. Conform to c89 (basically put any variable declarations at the top of a code block, not sprinkled throughout)
  3. CARES_USE_LIBRESOLV should be fixed to be defined via the autotools and cmake build systems rather than doing this in the code directly ... if you search for other usages of CARES_USE_LIBRESOLV if its set it disables other sections of code you're leaving enabled (unless this is really intentional).

Then a question ... why the default_malloc override? Are there actually instances where you've seen c-ares call malloc(0) and it matter?

@dhrumilrana
Copy link
Contributor Author

dhrumilrana commented Dec 14, 2020

looks pretty good. Just need a couple of fixups.

  1. Remove the assert
  2. Conform to c89 (basically put any variable declarations at the top of a code block, not sprinkled throughout)
  3. CARES_USE_LIBRESOLV should be fixed to be defined via the autotools and cmake build systems rather than doing this in the code directly ... if you search for other usages of CARES_USE_LIBRESOLV if its set it disables other sections of code you're leaving enabled (unless this is really intentional).

Then a question ... why the default_malloc override? Are there actually instances where you've seen c-ares call malloc(0) and it matter?

Yes, on z/OS malloc(0) returns 0, while on others it returns a valid pointer. As it is used in the linked list overriding it was safer as the other platforms would not be affected and this also allows future code to be added without worrying about z/OS functionality.

Also for CARES_USE_LIBRESOV, I have added a check for ZOS in CMakeLists.txt as per your request, but I'm confused as to how to add a check for z/OS in configure.ac where CARES_USE_LIBRESOV is being set for apple. Could you help out with it?

@bagder
Copy link
Member

bagder commented Dec 14, 2020

malloc(0) is documented to return either NULL or a pointer so no code in c-ares should depend on one or the other. in fact, malloc(0) should rather be completely avoided, IMHO.

Copy link
Member

@bradh352 bradh352 left a comment

Choose a reason for hiding this comment

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

more detailed review

src/lib/ares_init.c Outdated Show resolved Hide resolved
src/lib/ares_init.c Outdated Show resolved Hide resolved
src/lib/ares_init.c Outdated Show resolved Hide resolved
src/lib/ares_init.c Outdated Show resolved Hide resolved
src/lib/ares_init.c Outdated Show resolved Hide resolved
src/lib/ares_library_init.c Outdated Show resolved Hide resolved
src/lib/ares_setup.h Show resolved Hide resolved
@bradh352
Copy link
Member

I haven't seen the CMakelists.txt modification, once that is added to this PR, I can then tell you what needs to happen in autotools to do the equivalent check...

src/lib/ares_init.c Outdated Show resolved Hide resolved
src/lib/ares_init.c Outdated Show resolved Hide resolved
@bradh352
Copy link
Member

Regarding autotools support for z/OS, I'd think in configure.ac after this:

dnl resolv lib for Apple (MacOS and iOS)
AS_IF([test "x$host_vendor" = "xapple"], [
  AC_SEARCH_LIBS([res_servicename], [resolv], [
    AC_DEFINE([CARES_USE_LIBRESOLV], [1], [Use resolver library to configure cares])
  ], [
    AC_MSG_ERROR([Unable to find libresolv which is required for iPhone targets])
  ])
])

you could use

dnl resolv lib for z/OS
AS_IF([test "x$host_vendor" = "xibm" -a "x$host_os" = "xopenedition" ], [
  AC_SEARCH_LIBS([res_init], [resolv], [
    AC_DEFINE([CARES_USE_LIBRESOLV], [1], [Use resolver library to configure cares])
  ], [
    AC_MSG_ERROR([Unable to find libresolv which is required for z/OS])
  ])
])

@dhrumilrana
Copy link
Contributor Author

dhrumilrana commented Dec 17, 2020

Added the autotools update. Regarding malloc(0) calls I see that malloc(0) is being called when in two of the node.js FVT test cases, I have linked the two tests: test 1 and test 2. Let me know if I need to add in the previous block of code.

@bradh352
Copy link
Member

Added the autotools update. Regarding malloc(0) calls I see that malloc(0) is being called when in two of the node.js FVT test cases, I have linked the two tests: test 1 and test 2. Let me know if I need to add in the previous block of code.

Can you be more specified in regards to the test failures? Is there a backtrace? Can you show what line in the tests they failed on? We'd need to know the code path if it is crashing due to this to know if its something in nodejs's assumptions or something deep inside c-ares.

@bradh352
Copy link
Member

Also, does the c-ares test suite work for you on z/OS the way it is right now (without the malloc workaround)?

I just performed a local hack to have ares_malloc(0) return NULL always and print out a message if this condition is hit. At least none of the conditions in the test suite appear to have hit this at all. That doesn't mean its not possible in some code path not in the test suite, of course.

@dhrumilrana
Copy link
Contributor Author

Yes, I ran the c-ares test-suite with and without the malloc workaround and there are no hits in either cases. I can provide you with a more detailed analysis of the failure soon.

@bradh352
Copy link
Member

I'm going to go ahead and merge this and treat the malloc(0) as a different issue. Infact, I'll probably follow-up with a commit to make the default malloc implementation return NULL on 0 always to catch any possible bugs. Maybe we can get a report of an issue on another system with NodeJS.

@bradh352 bradh352 merged commit 3c0bc07 into c-ares:master Dec 24, 2020
sergepetrenko pushed a commit to tarantool/c-ares that referenced this pull request Jul 29, 2022
Port c-ares to z/OS.

Fix By: Dhrumil Rana (@dhrumilrana)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants