Details

    • Patch Info:
      Patch Available

      Description

      Create a usable implementation of Thrift that uses only C at runtime, no C++. The code is at http://svn.apache.org/repos/asf/incubator/thrift/branches/c-bindings/.

      1. c_glib_bindings.patch
        290 kB
        Michael Lum
      2. t_c_generator.cc
        102 kB
        Michael Lum
      3. thrift_582_c_glib_bindings_20100927.patch
        397 kB
        Michael Lum
      4. thrift_582_c_glib_bindings_20101012.patch
        402 kB
        Anatol Pomozov
      5. thrift_582_c_glib_bindings_20101026.patch
        395 kB
        Anatol Pomozov
      6. thrift_582_c_glib_bindings.patch
        449 kB
        Michael Lum

        Issue Links

          Activity

          Hide
          David Reiss added a comment -

          The code doesn't appear to have been modified since Ross's last patch inGit: http://thrift-rpc.org/?p=thrift.git;a=shortlog;h=refs/heads/pri/rmcfarland/c_bindings_linear

          Show
          David Reiss added a comment - The code doesn't appear to have been modified since Ross's last patch inGit: http://thrift-rpc.org/?p=thrift.git;a=shortlog;h=refs/heads/pri/rmcfarland/c_bindings_linear
          Hide
          Michael Lum added a comment -

          working on this here: http://github.com/mlum/thrift

          Show
          Michael Lum added a comment - working on this here: http://github.com/mlum/thrift
          Hide
          Brandon Bickford added a comment -

          Any progress on this?

          Show
          Brandon Bickford added a comment - Any progress on this?
          Hide
          Michael Lum added a comment -

          here's the summary

          • the thrift binary protocol is implemented
          • the buffered, framed, memory buffer, socket, and server socket transports are implemented
          • the codegen adds a --gen c option that supports all Thrift types as various glib types
          • there are hooks into autotools to support gcov and valgrind to show coverage and memory/leak checks
          • about 25 tests
          • test that calls all services in ThriftTest.thrift from a C client to a C++ server
          • have a working cassandra 0.6.2 client
          • compiles on Snow Leopard, CentOS 5 and MinGW in WinXP

          what's left:

          • still working on getting code coverage up to 100% with no memory leaks reported by valgrind.
          • some additional thrift server code to support C services

          both are in progress.

          Show
          Michael Lum added a comment - here's the summary the thrift binary protocol is implemented the buffered, framed, memory buffer, socket, and server socket transports are implemented the codegen adds a --gen c option that supports all Thrift types as various glib types there are hooks into autotools to support gcov and valgrind to show coverage and memory/leak checks about 25 tests test that calls all services in ThriftTest.thrift from a C client to a C++ server have a working cassandra 0.6.2 client compiles on Snow Leopard, CentOS 5 and MinGW in WinXP what's left: still working on getting code coverage up to 100% with no memory leaks reported by valgrind. some additional thrift server code to support C services both are in progress.
          Hide
          Bryan Duxbury added a comment -

          Is it worth getting this into the codebase either in a branch or trunk as an "experimental" language? There's a good chance it will be useful to some folks before its "stable".

          Show
          Bryan Duxbury added a comment - Is it worth getting this into the codebase either in a branch or trunk as an "experimental" language? There's a good chance it will be useful to some folks before its "stable".
          Hide
          David Reiss added a comment -

          This is already in a branch at http://svn.apache.org/repos/asf/incubator/thrift/branches/c-bindings/

          I'd be okay moving it into trunk, but two things I think we should definitely do first are (1) delete random files like "gen-c" at top level and (2) rename it to c_glib or something like that to indicate that the implementation is very glib-dependent.

          Show
          David Reiss added a comment - This is already in a branch at http://svn.apache.org/repos/asf/incubator/thrift/branches/c-bindings/ I'd be okay moving it into trunk, but two things I think we should definitely do first are (1) delete random files like "gen-c" at top level and (2) rename it to c_glib or something like that to indicate that the implementation is very glib-dependent.
          Hide
          Michael Lum added a comment -

          the implementation doesn't have any gen-c at top level, its a pretty big refactoring of what was there.

          i'll submit a patch soon after making some changes:

          • applies cleanly to trunk
          • rename to --gen c_glib
          • makes c_glib optional
          Show
          Michael Lum added a comment - the implementation doesn't have any gen-c at top level, its a pretty big refactoring of what was there. i'll submit a patch soon after making some changes: applies cleanly to trunk rename to --gen c_glib makes c_glib optional
          Hide
          David Reiss added a comment -

          > the implementation doesn't have any gen-c at top level
          The branch I pasted appears to:

          dreiss@dr-mbp:master:dreiss:0$ svn ls http://svn.apache.org/repos/asf/incubator/thrift/branches/c-bindings/ | grep gen
          gen-c/
          
          Show
          David Reiss added a comment - > the implementation doesn't have any gen-c at top level The branch I pasted appears to: dreiss@dr-mbp:master:dreiss:0$ svn ls http://svn.apache.org/repos/asf/incubator/thrift/branches/c-bindings/ | grep gen gen-c/
          Hide
          Michael Lum added a comment -

          Sorry, I meant the patch I plan on submitting, that is currently on github, does not have gen-c at the top-level, and will apply cleanly to trunk.

          Show
          Michael Lum added a comment - Sorry, I meant the patch I plan on submitting, that is currently on github, does not have gen-c at the top-level, and will apply cleanly to trunk.
          Hide
          Michael Lum added a comment -

          OK, here it is.

          core files that were touched:

          • configure.ac - adding C checks, coverage hooks, valgrind hooks, etc.
          • compiler/cpp/src/parse/t_program.h - added function for C includes
          • compiler/cpp/Makefile.am - added THRIFT_GEN_c conditional
          • test/Makefile.am - conditional to add c subdir
          • lib/Makefile.am - conditional to add c subdir
          • .gitignore - for new generated files
          • test/DebugProtoTest.thrift - added C namespace for tests
          • test/ThriftTest.thrift - added C namespace for tests
          • test/OptionalRequiredTest.thrift - added C namespace for tests

          added files

          • compiler/cpp/src/generate/t_c_generator.cc
          • lib/c/*
          • test/c/*

          Note that the library does not build by default (although the code generator does), so to try this you have to run:

          ./configure --with-c ; make ; make check

          all tests pass on Snow Leopard and CentOS 5 x86_64 for me.

          All the to-do items from this ticket are still outstanding, as well as adding any necessary ASF license headers.

          Show
          Michael Lum added a comment - OK, here it is. core files that were touched: configure.ac - adding C checks, coverage hooks, valgrind hooks, etc. compiler/cpp/src/parse/t_program.h - added function for C includes compiler/cpp/Makefile.am - added THRIFT_GEN_c conditional test/Makefile.am - conditional to add c subdir lib/Makefile.am - conditional to add c subdir .gitignore - for new generated files test/DebugProtoTest.thrift - added C namespace for tests test/ThriftTest.thrift - added C namespace for tests test/OptionalRequiredTest.thrift - added C namespace for tests added files compiler/cpp/src/generate/t_c_generator.cc lib/c/* test/c/* Note that the library does not build by default (although the code generator does), so to try this you have to run: ./configure --with-c ; make ; make check all tests pass on Snow Leopard and CentOS 5 x86_64 for me. All the to-do items from this ticket are still outstanding, as well as adding any necessary ASF license headers.
          Hide
          David Reiss added a comment -

          It looks like the generator is not in the patch. You can just submit the output of git diff if you want. Would you mind renaming the generator and lib directory to c_glib?

          Show
          David Reiss added a comment - It looks like the generator is not in the patch. You can just submit the output of git diff if you want. Would you mind renaming the generator and lib directory to c_glib?
          Hide
          Michael Lum added a comment -

          forgot this file.

          Show
          Michael Lum added a comment - forgot this file.
          Hide
          Michael Lum added a comment -

          what specifically do you think should be "c_glib"? I attempted to change it in a few places, and some of the built-in macros and build artifacts didn't like having underscores in the generator name. IIRC, THRIFT_REGISTER_GENERATOR was one of them. I didn't dig deeper into the macros themselves. Unless it can be called cglib (which seems even more confusing to me).

          Show
          Michael Lum added a comment - what specifically do you think should be "c_glib"? I attempted to change it in a few places, and some of the built-in macros and build artifacts didn't like having underscores in the generator name. IIRC, THRIFT_REGISTER_GENERATOR was one of them. I didn't dig deeper into the macros themselves. Unless it can be called cglib (which seems even more confusing to me).
          Hide
          David Reiss added a comment -

          > what specifically do you think should be "c_glib"?
          The generator file and generator class, the library directory, and the first argument to THRIFT_REGISTER_GENERATOR. Basically, anything that would prevent us from having an additional C implementation using a different utility library (like apr).

          I don't see anything in THRIFT_REGISTER_GENERATOR that would cause a problem, because "_" is a valid identifier character. Do you remember the error you got?

          Show
          David Reiss added a comment - > what specifically do you think should be "c_glib"? The generator file and generator class, the library directory, and the first argument to THRIFT_REGISTER_GENERATOR. Basically, anything that would prevent us from having an additional C implementation using a different utility library (like apr). I don't see anything in THRIFT_REGISTER_GENERATOR that would cause a problem, because "_" is a valid identifier character. Do you remember the error you got?
          Hide
          Michael Lum added a comment -

          Let me take a stab at changing the names to reproduce the errors I got. I can't remember exactly what it was. Either that or we take a vote for namespace squatter's rights

          Show
          Michael Lum added a comment - Let me take a stab at changing the names to reproduce the errors I got. I can't remember exactly what it was. Either that or we take a vote for namespace squatter's rights
          Hide
          Michael Lum added a comment -

          This patch should change the generator to c_glib, and moves the libraries to c_glib.

          Show
          Michael Lum added a comment - This patch should change the generator to c_glib, and moves the libraries to c_glib.
          Hide
          Michael Lum added a comment -

          Any feedback on the updated patch? I'm guessing it won't cleanly apply to trunk anymore since it's been two weeks, but if the general idea looks correct, I can update it to apply cleanly to the current trunk.

          Show
          Michael Lum added a comment - Any feedback on the updated patch? I'm guessing it won't cleanly apply to trunk anymore since it's been two weeks, but if the general idea looks correct, I can update it to apply cleanly to the current trunk.
          Hide
          David Reiss added a comment -

          I'll try to take a look over the weekend.

          Show
          David Reiss added a comment - I'll try to take a look over the weekend.
          Hide
          David Reiss added a comment -

          Based on my limited knowledge of glib, this looks good overall. I have a few comments:

          configure.ac:
          I'm pretty sure autoscan is incorrect to request PROG_AWK and PROG_RANLIB.
          Can you remove those?

          Can you update the c_glib naming on AX_THRIFT_LIB and AX_THRIFT_GEN?
          Also, can you add the c_glib binding to the summary printed out at the bottom?
          I changed the style of the library checks a bit to make this work.

          What is the source of the code used to enable coverage?

          test/c:
          Can you rename this to test/c_glib?

          Makefile.in:
          This should not be in the patch.

          thrift_binary_protocol.c:
          I think this line might cause problems with strict aliasing. See bitwise_cast in the C++ codebase for more information.
          guint64 bits = GUINT64_FROM_BE (*(unsigned long long *)&value);

          Makefile.am:
          no need for ACLOCAL_AMFLAGS anymore
          Replace thriftc with thrift_c_glib

          thriftc.pc:
          Replace thriftc with thrift_c_glib

          Show
          David Reiss added a comment - Based on my limited knowledge of glib, this looks good overall. I have a few comments: configure.ac: I'm pretty sure autoscan is incorrect to request PROG_AWK and PROG_RANLIB. Can you remove those? Can you update the c_glib naming on AX_THRIFT_LIB and AX_THRIFT_GEN? Also, can you add the c_glib binding to the summary printed out at the bottom? I changed the style of the library checks a bit to make this work. What is the source of the code used to enable coverage? test/c: Can you rename this to test/c_glib? Makefile.in: This should not be in the patch. thrift_binary_protocol.c: I think this line might cause problems with strict aliasing. See bitwise_cast in the C++ codebase for more information. guint64 bits = GUINT64_FROM_BE (*(unsigned long long *)&value); Makefile.am: no need for ACLOCAL_AMFLAGS anymore Replace thriftc with thrift_c_glib thriftc.pc: Replace thriftc with thrift_c_glib
          Hide
          Anatol Pomozov added a comment -

          I think about adding Vala language support to Thrift https://issues.apache.org/jira/browse/THRIFT-764 and I need c-binding (with glib) for it. What is ETA for seeing it in the trunk?

          Once it will be added to trunk I start working on Vala support and BTW it will be a good test case for c-bindings (+glib).

          Anyways thanks Michael and everyone else for doing this great job.

          Show
          Anatol Pomozov added a comment - I think about adding Vala language support to Thrift https://issues.apache.org/jira/browse/THRIFT-764 and I need c-binding (with glib) for it. What is ETA for seeing it in the trunk? Once it will be added to trunk I start working on Vala support and BTW it will be a good test case for c-bindings (+glib). Anyways thanks Michael and everyone else for doing this great job.
          Hide
          Michael Lum added a comment -

          The changes aren't too much work to implement. I just haven't been able to get to them due to some other projects going on. I should at least be able to get it done in the next couple of weeks.

          Show
          Michael Lum added a comment - The changes aren't too much work to implement. I just haven't been able to get to them due to some other projects going on. I should at least be able to get it done in the next couple of weeks.
          Hide
          Michael Lum added a comment -

          This patch applies cleanly to r1001921.

          changes:

          • configure.ac removes PROG_AWK and PROG_RANLIB, although now bootstrap complains about it being needed by Erlang
          • renamed AX_THRIFT_LIB and AX_THRIFT_GEN naming to c_glib
          • the source of the code coverage are hooks into gcov, as seen in configure.ac and executed by the test-wrapper script in test/c_glib
          • Makefile.in removed
          • changed thrift_binary_protocol.c to use type punning to solve the strict aliasing problem
          • removed ACLOCAL_AMFLAGS
          • renamed thriftc to thrift_c_glib
          Show
          Michael Lum added a comment - This patch applies cleanly to r1001921. changes: configure.ac removes PROG_AWK and PROG_RANLIB, although now bootstrap complains about it being needed by Erlang renamed AX_THRIFT_LIB and AX_THRIFT_GEN naming to c_glib the source of the code coverage are hooks into gcov, as seen in configure.ac and executed by the test-wrapper script in test/c_glib Makefile.in removed changed thrift_binary_protocol.c to use type punning to solve the strict aliasing problem removed ACLOCAL_AMFLAGS renamed thriftc to thrift_c_glib
          Hide
          Anatol Pomozov added a comment -

          Michael, is there any git repo (github preferably) that contains your patches rebased on top of recent HEAD?

          Show
          Anatol Pomozov added a comment - Michael, is there any git repo (github preferably) that contains your patches rebased on top of recent HEAD?
          Hide
          Michael Lum added a comment -

          I was working on it here:
          http://github.com/mlum/thrift

          However, it is out of date. I've been locally merging it with the trunk to produce patches that apply cleanly to trunk. I was hoping that this would get included into the thrift trunk soon so that I could rebase on it and keep working on features.

          Since 10 days have passed since I submitted the last patch, I'm guessing it won't cleanly apply to the trunk anymore and a new patch will have to be created that works.

          Show
          Michael Lum added a comment - I was working on it here: http://github.com/mlum/thrift However, it is out of date. I've been locally merging it with the trunk to produce patches that apply cleanly to trunk. I was hoping that this would get included into the thrift trunk soon so that I could rebase on it and keep working on features. Since 10 days have passed since I submitted the last patch, I'm guessing it won't cleanly apply to the trunk anymore and a new patch will have to be created that works.
          Hide
          Anatol Pomozov added a comment -

          Hi, Michael.

          If it is not very hard for you, could you please push your recent changes (+merge from trunk) to http://github.com/mlum/thrift

          Thanks for what you are doing.

          Show
          Anatol Pomozov added a comment - Hi, Michael. If it is not very hard for you, could you please push your recent changes (+merge from trunk) to http://github.com/mlum/thrift Thanks for what you are doing.
          Hide
          Anatol Pomozov added a comment -

          Rebase previous path to Thrift HEAD. Fix 'make check'.

          Show
          Anatol Pomozov added a comment - Rebase previous path to Thrift HEAD. Fix 'make check'.
          Hide
          Bryan Duxbury added a comment -

          What's the consensus on this? I'd love to commit it so you guys can stop rebasing it all the time, but I lack the personal experience to give it a review.

          Show
          Bryan Duxbury added a comment - What's the consensus on this? I'd love to commit it so you guys can stop rebasing it all the time, but I lack the personal experience to give it a review.
          Hide
          David Reiss added a comment -

          All of my concerns have been addressed. I'm okay with the patch at this point. I think last patch from Anatol Pomozov might have broken test-wrapper.sh by moving it out of AC_CONFIG_FILES.

          Show
          David Reiss added a comment - All of my concerns have been addressed. I'm okay with the patch at this point. I think last patch from Anatol Pomozov might have broken test-wrapper.sh by moving it out of AC_CONFIG_FILES.
          Hide
          Anatol Pomozov added a comment -

          Hi, David.

          You are right. I added it back to the list. See fix here http://github.com/anatol/thrift/commit/4c5aa31f04a0ab6addbb9a8dea6d8bfe3c171288

          Also I think we need to add *.h to the list of INCLUDE files http://github.com/anatol/thrift/commit/bccb67878b305f03fc175e13eb29201f249d72ce

          Show
          Anatol Pomozov added a comment - Hi, David. You are right. I added it back to the list. See fix here http://github.com/anatol/thrift/commit/4c5aa31f04a0ab6addbb9a8dea6d8bfe3c171288 Also I think we need to add *.h to the list of INCLUDE files http://github.com/anatol/thrift/commit/bccb67878b305f03fc175e13eb29201f249d72ce
          Hide
          Roger Meier added a comment -

          great job!

          I think there are some minor issues to be solved before commit:

          • I would like to see language specific tests in their appropriate library directory THRIFT-35 (few languages are done, its easy to do for new ones)
          • all unit test should pass without error
          • It seems, that at least the unit tests depend on cpp implementation, what about adding a dependancy on cpp inside configure.ac to ensure that we have with-cpp?
           ./configure --without-csharp --without-java --without-erlang --without-perl --without-php --without-php_extension --without-ruby --without-haskell --without-python --without-cpp --with-c_glib 
          .....
          g++ -DHAVE_CONFIG_H -I. -I../..  -I../../lib/cpp/src  -I./gen-cpp -I../../lib/c_glib/src -I./gen-c -I/usr/include/glib-2.0 -I/usr/lib/glib-2.0/include     -g -MT testthrifttestclient-testthrifttestclient.o -MD -MP -MF .deps/testthrifttestclient-testthrifttestclient.Tpo -c -o testthrifttestclient-testthrifttestclient.o `test -f 'testthrifttestclient.cpp' || echo './'`testthrifttestclient.cpp
          mv -f .deps/testthrifttestclient-testthrifttestclient.Tpo .deps/testthrifttestclient-testthrifttestclient.Po
          make[4]: *** No rule to make target `../../lib/cpp/.libs/libthrift.la', needed by `testthrifttestclient'.  Stop.
          make[4]: Leaving directory `/home/roger/software/thrift/thrift-trunk-c_glib/test/c_glib'
          

          after configuring --with-cpp it nearly passes all unit tests.. but the following 2 tests fail:

          PASS: testdebugproto
          /bin/sh: line 4: 21597 Segmentation fault      ${dir}$tst
          FAIL: testoptionalrequired
          ....
          PASS: testwrapper-testdebugproto
          ./testwrapper-testoptionalrequired: line 7: 21888 Segmentation fault      "$stripcommand" "$@"
          FAIL: testwrapper-testoptionalrequired
          

          My Environment is: Debian GNU/Linux Lenny with

          ii  libglib2.0-0                         2.16.6-3                   The GLib library of C routines
          ii  libglib2.0-dev                       2.16.6-3                   Development files for the GLib library
          ii  gcc                                  4:4.3.2-2                  The GNU C compiler
          ii  gcc-4.3                              4.3.2-1.1                  The GNU C compiler
          ii  gcc-4.3-base                         4.3.2-1.1                  The GNU Compiler Collection (base package)
          ii  libtool                              1.5.26-4+lenny1            Generic library support script
          
          Show
          Roger Meier added a comment - great job! I think there are some minor issues to be solved before commit: I would like to see language specific tests in their appropriate library directory THRIFT-35 (few languages are done, its easy to do for new ones) all unit test should pass without error It seems, that at least the unit tests depend on cpp implementation, what about adding a dependancy on cpp inside configure.ac to ensure that we have with-cpp? ./configure --without-csharp --without-java --without-erlang --without-perl --without-php --without-php_extension --without-ruby --without-haskell --without-python --without-cpp --with-c_glib ..... g++ -DHAVE_CONFIG_H -I. -I../.. -I../../lib/cpp/src -I./gen-cpp -I../../lib/c_glib/src -I./gen-c -I/usr/include/glib-2.0 -I/usr/lib/glib-2.0/include -g -MT testthrifttestclient-testthrifttestclient.o -MD -MP -MF .deps/testthrifttestclient-testthrifttestclient.Tpo -c -o testthrifttestclient-testthrifttestclient.o `test -f 'testthrifttestclient.cpp' || echo './'`testthrifttestclient.cpp mv -f .deps/testthrifttestclient-testthrifttestclient.Tpo .deps/testthrifttestclient-testthrifttestclient.Po make[4]: *** No rule to make target `../../lib/cpp/.libs/libthrift.la', needed by `testthrifttestclient'. Stop. make[4]: Leaving directory `/home/roger/software/thrift/thrift-trunk-c_glib/test/c_glib' after configuring --with-cpp it nearly passes all unit tests.. but the following 2 tests fail: PASS: testdebugproto /bin/sh: line 4: 21597 Segmentation fault ${dir}$tst FAIL: testoptionalrequired .... PASS: testwrapper-testdebugproto ./testwrapper-testoptionalrequired: line 7: 21888 Segmentation fault "$stripcommand" "$@" FAIL: testwrapper-testoptionalrequired My Environment is: Debian GNU/Linux Lenny with ii libglib2.0-0 2.16.6-3 The GLib library of C routines ii libglib2.0-dev 2.16.6-3 Development files for the GLib library ii gcc 4:4.3.2-2 The GNU C compiler ii gcc-4.3 4.3.2-1.1 The GNU C compiler ii gcc-4.3-base 4.3.2-1.1 The GNU Compiler Collection (base package ) ii libtool 1.5.26-4+lenny1 Generic library support script
          Hide
          Anatol Pomozov added a comment -

          Test files are moved to lib/c_glib/test. Check this out http://github.com/anatol/thrift/commit/aa205295358bc141010cb9773b5535c0b2f3fdd4

          ./configure --with-c-glib --without-ruby --without-python --without-erlang
          make
          make check

          works fine for me.

          Regarding CPP file - this is a test that checks possibility of using C client with C++ server. The best thing as for me is to add "if WITH_CPP" to Makefile for this particular test.

          I also found another issue: the output dir for the generator is "gen-c", it should be "gen-c_glib" instead. Does it sound right?

          PS I'll be unavailable next 5 days, so I cant respond your requests. If Michael wants to continue it i'll be glad. Otherwise i'll fix it next weekend.

          Show
          Anatol Pomozov added a comment - Test files are moved to lib/c_glib/test. Check this out http://github.com/anatol/thrift/commit/aa205295358bc141010cb9773b5535c0b2f3fdd4 ./configure --with-c-glib --without-ruby --without-python --without-erlang make make check works fine for me. Regarding CPP file - this is a test that checks possibility of using C client with C++ server. The best thing as for me is to add "if WITH_CPP" to Makefile for this particular test. I also found another issue: the output dir for the generator is "gen-c", it should be "gen-c_glib" instead. Does it sound right? PS I'll be unavailable next 5 days, so I cant respond your requests. If Michael wants to continue it i'll be glad. Otherwise i'll fix it next weekend.
          Hide
          Bryan Duxbury added a comment -

          Roger - can you see this one through to commit? Probably want to assign it to Anatol when it's committed though to track the contribution.

          Show
          Bryan Duxbury added a comment - Roger - can you see this one through to commit? Probably want to assign it to Anatol when it's committed though to track the contribution.
          Hide
          Roger Meier added a comment -

          Yes, I'm ready to do this Bryan.

          Anatol, I agree with you on these points:

          • The best thing as for me is to add "if WITH_CPP" to Makefile for this particular test.
          • rename gen-c folder to gen-c_glib for consistency

          just did a test with your version from git repo, still have the same issue on my Debian Lenny...see

          [New Thread 0xb74406b0 (LWP 20985)]
          [Switching to Thread 0xb74406b0 (LWP 20985)]
          
          Breakpoint 1, test_tricky2 () at testoptionalrequired.c:106
          106	  TTestTricky1 *t1 = NULL;
          (gdb) b testoptionalrequired.c:113
          Breakpoint 2 at 0x804e700: file testoptionalrequired.c, line 113.
          (gdb) c
          Continuing.
          
          Breakpoint 2, test_tricky2 () at testoptionalrequired.c:113
          113	  write_to_read (THRIFT_STRUCT (t3), THRIFT_STRUCT (t1), NULL, NULL);
          (gdb) b src/protocol/thrift_binary_protocol.c:148
          Breakpoint 3 at 0xb75ee01d: file src/protocol/thrift_binary_protocol.c, line 148.
          (gdb) c
          Continuing.
          
          Breakpoint 3, thrift_binary_protocol_get_type () at src/protocol/thrift_binary_protocol.c:148
          148	  if (type == 0)
          (gdb) 
          Continuing.
          
          Program received signal SIGSEGV, Segmentation fault.
          0xb76bc3dc in ?? () from /usr/lib/libgobject-2.0.so.0
          (gdb) 
          

          seems, that I have to become familiar with GType and GObject

          Show
          Roger Meier added a comment - Yes, I'm ready to do this Bryan. Anatol, I agree with you on these points: The best thing as for me is to add "if WITH_CPP" to Makefile for this particular test. rename gen-c folder to gen-c_glib for consistency just did a test with your version from git repo, still have the same issue on my Debian Lenny...see [New Thread 0xb74406b0 (LWP 20985)] [Switching to Thread 0xb74406b0 (LWP 20985)] Breakpoint 1, test_tricky2 () at testoptionalrequired.c:106 106 TTestTricky1 *t1 = NULL; (gdb) b testoptionalrequired.c:113 Breakpoint 2 at 0x804e700: file testoptionalrequired.c, line 113. (gdb) c Continuing. Breakpoint 2, test_tricky2 () at testoptionalrequired.c:113 113 write_to_read (THRIFT_STRUCT (t3), THRIFT_STRUCT (t1), NULL, NULL); (gdb) b src/protocol/thrift_binary_protocol.c:148 Breakpoint 3 at 0xb75ee01d: file src/protocol/thrift_binary_protocol.c, line 148. (gdb) c Continuing. Breakpoint 3, thrift_binary_protocol_get_type () at src/protocol/thrift_binary_protocol.c:148 148 if (type == 0) (gdb) Continuing. Program received signal SIGSEGV, Segmentation fault. 0xb76bc3dc in ?? () from /usr/lib/libgobject-2.0.so.0 (gdb) seems, that I have to become familiar with GType and GObject
          Hide
          Michael Lum added a comment -

          I'll have a look at the segfault, since I probably created it.

          Show
          Michael Lum added a comment - I'll have a look at the segfault, since I probably created it.
          Hide
          Anatol Pomozov added a comment -

          Hi, Roger.

          Please check my github repo. I fixed 2 issues you mentioned. Please pull my changes and review it once again.

          I can't repro SEGFAULT neither on Ubuntu 10.04 nor on MacOSX 10.6. I don't understand why it fails. Michael, do you have any ideas?

          Show
          Anatol Pomozov added a comment - Hi, Roger. Please check my github repo. I fixed 2 issues you mentioned. Please pull my changes and review it once again. I can't repro SEGFAULT neither on Ubuntu 10.04 nor on MacOSX 10.6. I don't understand why it fails. Michael, do you have any ideas?
          Hide
          Michael Lum added a comment -

          Yeah, I wasn't able to reproduce it on an Ubuntu VM. When I get some time, I'm thinking of building a Debian Lenny VM, but the next two weeks are going to be a little busy for me.

          Show
          Michael Lum added a comment - Yeah, I wasn't able to reproduce it on an Ubuntu VM. When I get some time, I'm thinking of building a Debian Lenny VM, but the next two weeks are going to be a little busy for me.
          Hide
          Anatol Pomozov added a comment -

          I believe that the SEGFAULT happens because of old packages (libglib2). On Ubuntu I have 2.24 on mac - 2.26.

          BTW I am going to add Vala support to Thrift later and minimum version for vala is glib 2.24

          Show
          Anatol Pomozov added a comment - I believe that the SEGFAULT happens because of old packages (libglib2). On Ubuntu I have 2.24 on mac - 2.26. BTW I am going to add Vala support to Thrift later and minimum version for vala is glib 2.24
          Hide
          Anatol Pomozov added a comment -

          Also Debian Lenny has Autoconf 2.61 http://packages.debian.org/lenny/autoconf

          While Thrift HEAD requires 2.65 or higher.

          Show
          Anatol Pomozov added a comment - Also Debian Lenny has Autoconf 2.61 http://packages.debian.org/lenny/autoconf While Thrift HEAD requires 2.65 or higher.
          Hide
          Michael Lum added a comment -

          Ah I see. Anything else we need to clean up?

          Show
          Michael Lum added a comment - Ah I see. Anything else we need to clean up?
          Hide
          Anatol Pomozov added a comment -

          I installed Debian Lenny to a virtual machine and tried to compile Thrift with c_glib support.

          As I mentioned above Thrift requires Autoconf 2.61. I updated system from lenny-backports and "make && make check" works fine for me without any SEGFAULTS.

          What I did

          apt-get install git-core libglib2.0-0-dev autoconf libtool libboost-dev bison flex make g++ pkg-config
          ./bootstrap.sh 
          ./configure --with-c_glib --without-cpp --without-python
          make
          make check

          I did the same for Debian Testing with the same result - tests are passed.

          So it makes me think that the problem is in incompatibility with older glib version.

          Roger, can you please install libglib2.0-0 from lenny-backports and let us know if you still have the issue.

          Show
          Anatol Pomozov added a comment - I installed Debian Lenny to a virtual machine and tried to compile Thrift with c_glib support. As I mentioned above Thrift requires Autoconf 2.61. I updated system from lenny-backports and "make && make check" works fine for me without any SEGFAULTS. What I did apt-get install git-core libglib2.0-0-dev autoconf libtool libboost-dev bison flex make g++ pkg-config ./bootstrap.sh ./configure --with-c_glib --without-cpp --without-python make make check I did the same for Debian Testing with the same result - tests are passed. So it makes me think that the problem is in incompatibility with older glib version. Roger, can you please install libglib2.0-0 from lenny-backports and let us know if you still have the issue.
          Hide
          Roger Meier added a comment -

          Great job! All 22 tests passed

          I did sudo apt-get -t lenny-backports install libglib2.0-dev on my Debian Lenny and it rocks!

          Could someone create the final patch against trunk and I will commit this.

          Show
          Roger Meier added a comment - Great job! All 22 tests passed I did sudo apt-get -t lenny-backports install libglib2.0-dev on my Debian Lenny and it rocks! Could someone create the final patch against trunk and I will commit this.
          Hide
          Anatol Pomozov added a comment -

          git diff -u apache/trunk

          Show
          Anatol Pomozov added a comment - git diff -u apache/trunk
          Hide
          Roger Meier added a comment -

          as proposed by Bryan, assigned to Anatol

          Show
          Roger Meier added a comment - as proposed by Bryan, assigned to Anatol
          Hide
          Roger Meier added a comment -

          just committed the latest patch with some additions to the svn:ignore properties.

          Thank you Anatol and Michael for that huge effort! Just GREAT!

          Show
          Roger Meier added a comment - just committed the latest patch with some additions to the svn:ignore properties. Thank you Anatol and Michael for that huge effort! Just GREAT!
          Hide
          Anatol Pomozov added a comment -

          Yay! Thanks everyone.

          One more thing - can you please remove https://svn.apache.org/viewvc/incubator/thrift/branches/c-bindings/ this branch is not needed anymore.

          Show
          Anatol Pomozov added a comment - Yay! Thanks everyone. One more thing - can you please remove https://svn.apache.org/viewvc/incubator/thrift/branches/c-bindings/ this branch is not needed anymore.
          Hide
          Roger Meier added a comment -

          c-bindings branch is deleted

          Show
          Roger Meier added a comment - c-bindings branch is deleted
          Hide
          Anatol Pomozov added a comment -

          Not sure why, but git official git mirror git://git.apache.org/thrift.git still contains c-bindings c-compiler branches. Sounds like a bug in svn->git conversion tool.

          Anyway it is not a blocker. I am going to close the issue.

          Show
          Anatol Pomozov added a comment - Not sure why, but git official git mirror git://git.apache.org/thrift.git still contains c-bindings c-compiler branches. Sounds like a bug in svn->git conversion tool. Anyway it is not a blocker. I am going to close the issue.
          Hide
          Anatol Pomozov added a comment -

          Changes are in svn now. Thanks everyone.

          Show
          Anatol Pomozov added a comment - Changes are in svn now. Thanks everyone.
          Hide
          Aurélien Revol added a comment -

          Hi,

          compiles on Snow Leopard, CentOS 5 and MinGW in WinXP

          Is the official thrift-0.6.1 version of the c_glib still compatible with MinGW?

          Under WinXP, MSys 1.0 with MinGW (GCC 3.4.5) and GTK+ 2.16, I do get

          ./configure --with-java=no --with-ruby=no --with-python=no --with-php=no --with-perl=no --with-cpp=no --with-c_glib=yes
          

          to work OK ( Building C (GLib) Library .... : yes ); however compilation fails. As I do:

          cd lib/c_glib ; make
          

          gcc returns the following errors:

           gcc -DHAVE_CONFIG_H -I. -I. -I../.. -g -Wall -W -Werror -Isrc -mms-bitfields -Ic:/GTK_2.16/include/glib-2.0 -Ic:/GTK_2.16/lib/glib-2.0/include -MT libthrift_c_glib_la-thrift_transport_factory.lo -MD -MP -MF .deps/libthrift_c_glib_la-thrift_transport_factory.Tpo -c src/transport/thrift_transport_factory.c  -DDLL_EXPORT -DPIC -o .libs/libthrift_c_glib_la-thrift_transport_factory.o
          src/transport/thrift_socket.c:21:19: netdb.h: No such file or directory
          src/transport/thrift_socket.c: In function `thrift_socket_open':
          src/transport/thrift_socket.c:56: error: storage size of 'pin' isn't known
          src/transport/thrift_socket.c:62: warning: implicit declaration of function `gethostbyname'
          src/transport/thrift_socket.c:62: warning: assignment makes pointer from integer without a cast
          src/transport/thrift_socket.c:68: warning: implicit declaration of function `hstrerror'
          src/transport/thrift_socket.c:68: error: `h_errno' undeclared (first use in this function)
          src/transport/thrift_socket.c:68: error: (Each undeclared identifier is reported only once
          src/transport/thrift_socket.c:68: error: for each function it appears in.)
          src/transport/thrift_socket.c:68: warning: format argument is not a pointer (arg 7)
          src/transport/thrift_socket.c:74: error: `AF_INET' undeclared (first use in this function)
          src/transport/thrift_socket.c:75: error: dereferencing pointer to incomplete type
          src/transport/thrift_socket.c:76: warning: implicit declaration of function `htons'
          src/transport/thrift_socket.c:79: warning: implicit declaration of function `socket'
          src/transport/thrift_socket.c:79: error: `SOCK_STREAM' undeclared (first use in this function)
          src/transport/thrift_socket.c:89: warning: implicit declaration of function `connect'
          src/transport/thrift_socket.c:56: warning: unused variable `pin'
          src/transport/thrift_socket.c: In function `thrift_socket_read':
          src/transport/thrift_socket.c:130: warning: implicit declaration of function `recv'
          src/transport/thrift_socket.c: In function `thrift_socket_write':
          src/transport/thrift_socket.c:168: warning: implicit declaration of function `send'
          make[1]: *** [libthrift_c_glib_la-thrift_socket.lo] Error 1
          

          Indeed I wonder how code looking for netdb.h could ever be compiled against the bare WIN32 platform (and for me Cygwin is not an option).

          Thanks for any help.

          Show
          Aurélien Revol added a comment - Hi, compiles on Snow Leopard, CentOS 5 and MinGW in WinXP Is the official thrift-0.6.1 version of the c_glib still compatible with MinGW? Under WinXP, MSys 1.0 with MinGW (GCC 3.4.5) and GTK+ 2.16, I do get ./configure --with-java=no --with-ruby=no --with-python=no --with-php=no --with-perl=no --with-cpp=no --with-c_glib=yes to work OK ( Building C (GLib) Library .... : yes ); however compilation fails. As I do: cd lib/c_glib ; make gcc returns the following errors: gcc -DHAVE_CONFIG_H -I. -I. -I../.. -g -Wall -W -Werror -Isrc -mms-bitfields -Ic:/GTK_2.16/include/glib-2.0 -Ic:/GTK_2.16/lib/glib-2.0/include -MT libthrift_c_glib_la-thrift_transport_factory.lo -MD -MP -MF .deps/libthrift_c_glib_la-thrift_transport_factory.Tpo -c src/transport/thrift_transport_factory.c -DDLL_EXPORT -DPIC -o .libs/libthrift_c_glib_la-thrift_transport_factory.o src/transport/thrift_socket.c:21:19: netdb.h: No such file or directory src/transport/thrift_socket.c: In function `thrift_socket_open': src/transport/thrift_socket.c:56: error: storage size of 'pin' isn't known src/transport/thrift_socket.c:62: warning: implicit declaration of function `gethostbyname' src/transport/thrift_socket.c:62: warning: assignment makes pointer from integer without a cast src/transport/thrift_socket.c:68: warning: implicit declaration of function `hstrerror' src/transport/thrift_socket.c:68: error: `h_errno' undeclared (first use in this function) src/transport/thrift_socket.c:68: error: (Each undeclared identifier is reported only once src/transport/thrift_socket.c:68: error: for each function it appears in.) src/transport/thrift_socket.c:68: warning: format argument is not a pointer (arg 7) src/transport/thrift_socket.c:74: error: `AF_INET' undeclared (first use in this function) src/transport/thrift_socket.c:75: error: dereferencing pointer to incomplete type src/transport/thrift_socket.c:76: warning: implicit declaration of function `htons' src/transport/thrift_socket.c:79: warning: implicit declaration of function `socket' src/transport/thrift_socket.c:79: error: `SOCK_STREAM' undeclared (first use in this function) src/transport/thrift_socket.c:89: warning: implicit declaration of function `connect' src/transport/thrift_socket.c:56: warning: unused variable `pin' src/transport/thrift_socket.c: In function `thrift_socket_read': src/transport/thrift_socket.c:130: warning: implicit declaration of function `recv' src/transport/thrift_socket.c: In function `thrift_socket_write': src/transport/thrift_socket.c:168: warning: implicit declaration of function `send' make[1]: *** [libthrift_c_glib_la-thrift_socket.lo] Error 1 Indeed I wonder how code looking for netdb.h could ever be compiled against the bare WIN32 platform (and for me Cygwin is not an option). Thanks for any help.
          Hide
          Aurélien Revol added a comment -

          Well I suppose this answers my questions: THRIFT-1016: using GSocket in c_glib library . (Another element of answer lies in the existence of THRIFT-1145, whose description is misleading however.)

          Has that ever been tried? What are the possible obstacles to using GSocket?

          Show
          Aurélien Revol added a comment - Well I suppose this answers my questions: THRIFT-1016: using GSocket in c_glib library . (Another element of answer lies in the existence of THRIFT-1145 , whose description is misleading however.) Has that ever been tried? What are the possible obstacles to using GSocket?
          Hide
          Michael Lum added a comment -

          there shouldn't be any obstacles to using GSocket, just some time to implement it.
          when i get some time i'll have a look at my msys/mingw environment

          Show
          Michael Lum added a comment - there shouldn't be any obstacles to using GSocket, just some time to implement it. when i get some time i'll have a look at my msys/mingw environment
          Hide
          Aurélien Revol added a comment -

          Thanks for the info. It's good to know there's this option.

          Show
          Aurélien Revol added a comment - Thanks for the info. It's good to know there's this option.
          Hide
          Christian Zimnick added a comment -

          I tested c_glib client with a csharp server just for fun.
          c_glib client calling a function on csharp-server and returns a big string.
          On any time i get a segfault or block by reading the returning string from server.

          /*
           *   calling example from my test client...
           *	segfault by reading the returning string from server
           */
          gchar *_return = NULL; /* found: allocation of string length in lib/c_glib/src/thrift_binary_protocol.c:757  ??allocate some space by myself?? */
          testServer_if_run_test_function(service, &_return, argument, &err);
          printf("after send |%s|\n", _return);
          

          c_glib client runs on Ubuntu Server 11.04.
          csharp server runs on Windows 7 64 Bit. (.NET)

          I fixed this with some code changes in lib/c_glib/src/transport/thrift_socket.c and lib/src/transport/thrift_buffered_transport.c. I hope its correct.

          Index: lib/c_glib/src/transport/thrift_socket.c
          ===================================================================
          --- lib/c_glib/src/transport/thrift_socket.c    (revision 1190015)
          +++ lib/c_glib/src/transport/thrift_socket.c    (working copy)
          @@ -127,7 +127,7 @@
          
             while (got < len)
             {
          -    ret = recv (socket->sd, buf, len, 0);
          +    ret = recv (socket->sd, buf+got, len-got, 0);
               if (ret < 0)
               {
                 g_set_error (error, THRIFT_TRANSPORT_ERROR,
          Index: lib/c_glib/src/transport/thrift_buffered_transport.c
          ===================================================================
          --- lib/c_glib/src/transport/thrift_buffered_transport.c    (revision 1190015)
          +++ lib/c_glib/src/transport/thrift_buffered_transport.c    (working copy)
          @@ -71,7 +71,7 @@
             ThriftBufferedTransport *t = THRIFT_BUFFERED_TRANSPORT (transport);
             guint32 want = len;
             guint32 got = 0;
          -  guchar tmpdata[t->r_buf_size];
          +  guchar tmpdata[len];
             guint32 have = t->r_buf->len;
          
             // we shouldn't hit this unless the buffer doesn't have enough to read
          @@ -97,14 +97,14 @@
          
               // copy the data starting from where we left off
               memcpy (buf + have, tmpdata, got);
          -    return got + have;
          +    return got + have;
             } else {
               got += THRIFT_TRANSPORT_GET_CLASS (t->transport)->read (t->transport,
                                                                       tmpdata,
          -                                                            t->r_buf_size,
          +                                                            want,
                                                                       error);
               t->r_buf = g_byte_array_append (t->r_buf, tmpdata, got);
          -
          +
               // hand over what we have up to what the caller wants
               guint32 give = want < t->r_buf->len ? want : t->r_buf->len;
          

          After this changes framed-transport and buffered-transport on binary-protocol worked fine.

          Show
          Christian Zimnick added a comment - I tested c_glib client with a csharp server just for fun. c_glib client calling a function on csharp-server and returns a big string. On any time i get a segfault or block by reading the returning string from server. /* * calling example from my test client... * segfault by reading the returning string from server */ gchar *_return = NULL; /* found: allocation of string length in lib/c_glib/src/thrift_binary_protocol.c:757 ??allocate some space by myself?? */ testServer_if_run_test_function(service, &_return, argument, &err); printf( "after send |%s|\n" , _return); c_glib client runs on Ubuntu Server 11.04. csharp server runs on Windows 7 64 Bit. (.NET) I fixed this with some code changes in lib/c_glib/src/transport/thrift_socket.c and lib/src/transport/thrift_buffered_transport.c. I hope its correct. Index: lib/c_glib/src/transport/thrift_socket.c =================================================================== --- lib/c_glib/src/transport/thrift_socket.c (revision 1190015) +++ lib/c_glib/src/transport/thrift_socket.c (working copy) @@ -127,7 +127,7 @@ while (got < len) { - ret = recv (socket->sd, buf, len, 0); + ret = recv (socket->sd, buf+got, len-got, 0); if (ret < 0) { g_set_error (error, THRIFT_TRANSPORT_ERROR, Index: lib/c_glib/src/transport/thrift_buffered_transport.c =================================================================== --- lib/c_glib/src/transport/thrift_buffered_transport.c (revision 1190015) +++ lib/c_glib/src/transport/thrift_buffered_transport.c (working copy) @@ -71,7 +71,7 @@ ThriftBufferedTransport *t = THRIFT_BUFFERED_TRANSPORT (transport); guint32 want = len; guint32 got = 0; - guchar tmpdata[t->r_buf_size]; + guchar tmpdata[len]; guint32 have = t->r_buf->len; // we shouldn't hit this unless the buffer doesn't have enough to read @@ -97,14 +97,14 @@ // copy the data starting from where we left off memcpy (buf + have, tmpdata, got); - return got + have; + return got + have; } else { got += THRIFT_TRANSPORT_GET_CLASS (t->transport)->read (t->transport, tmpdata, - t->r_buf_size, + want, error); t->r_buf = g_byte_array_append (t->r_buf, tmpdata, got); - + // hand over what we have up to what the caller wants guint32 give = want < t->r_buf->len ? want : t->r_buf->len; After this changes framed-transport and buffered-transport on binary-protocol worked fine.

            People

            • Assignee:
              Anatol Pomozov
              Reporter:
              David Reiss
            • Votes:
              1 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development