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

Several changes.master #73

Open
wants to merge 17 commits into
base: master
Choose a base branch
from

Conversation

dilyanpalauzov
Copy link
Contributor

I have done these changes locally over the last year and would be glad, if they are integrated on the master branch.

When a searched string was found in a body part, and that was not the
last one, the loop terminated and no free(var[y]) was called on the
remaining bodyparts.

There is still a memory leak, if bc_compile_regex() fails and
goto alldone; is executed.

-- don't check on each iteration if val is non-NULL, but only at the
   beginning of the loop (as the loop does not change it)
Then the recipient is "" in sieve/bc_eval.c:eval_bc_test():case BC_ENVELOPE
after address_itr_init(&ai, "") the return value of address_itr_next(&ai)
is NULL, as tested in cunit/parseaddr.testc:test_iterator().

Even in this case, the while loop has to be entered for exactly one iteration.
Instead of keeping source files in EXTRA_DIST, that are build using nodist_
prefix, mention the files in (dist_).._SOURCES.

imap/lmtpstats.c, imap/pushstatc.s and imap/rfc822_header.c were already in
imap_(imapd,lmtpd,libcyrus_imap)_SOURCES, so there is no point to repeat
them in EXTRA_DIST.

Mention the man pages always in EXTRA_DIST instead of adding them sometimes
to EXTRA_DIST and sometimes to dist_man_MANS, and rename the latter occurrences
to man_MANS.

Include imap/imap_err.[ch] in the distribution, as it is done for
(mupdate,tz,nttp)_err.[ch] .
-- reorder alphabetically
-- add some symbols as EXPORTED, so that the corresponding files can be added
   to libcyrus_imap and not repeated as source files to httpd
.. so that it can be linked with libcyrus, even if the latter is a
shared object
.. so that .o files in that directory are not included in the
distribution tarball.
imap/message.h: I need this to call message_parse_mapped outside of cyrus
imap/mailbox.h: needed by imap/message.h
lib/byteorder64.h, imap/message_guid.h, imap/quota.h and imap/sequence.h:
   needed by imap/mailbox.h
lib/prot.h: needed by imap/message.h
sieve/varlist.h: as it is needed by sieve_interface.h
-- reorder alphabetically EXTRA_DIST=contrib/*
@elliefm
Copy link
Contributor

elliefm commented Nov 14, 2016

Github makes it difficult to review pull requests with many unrelated changes in them. Are you able to separate this into a few different pull requests, organised by the general thing being changed?

  • The pkgconfig (.pc) changes look fine to me (ac0b073 and 9addc2b), and I would happily merge them as is, but can't because of everything else currently bundled in
  • Likewise, the small fixes also look okay (3baf8c1, fce3ba9, 2269cdf, 0d61c72, bbbe4f9) and I'd be happy to merge them if not for everything else
  • The sieve changes would make a good separate pull request
  • The changes to what's installed would make a good separate pull request
  • The changes to what's distributed would make a good separate pull request
  • The change to how libcyrus_imap and httpd are built/linked (8bf5143) should be its own separate pull request

I'm assuming you have enough familiarity with git and github to manage this, but please ask if you need help :)

Finally, the change to make libcrc32 a libtool library (2acc922) completely defeats the purpose of libcrc32, and will not be merged.

libcrc32.a is a static library for the sole purpose of forcing the lib/crc32.c and lib/crc32c.c files to be compiled with -O3 optimisations, without forcing these optimisations on Cyrus as a whole. It is only a "library" because that's how one applies different compile flags for particular sources with automake; it is not intended to be used as a library in the general libtool sense. Yes, it produces non-portability warnings during linking, but that's fine. If you experience problems as a result of this, please raise an issue describing them.

@elliefm elliefm mentioned this pull request Nov 14, 2016
@dilyanpalauzov
Copy link
Contributor Author

When libcrc32.a is compiled statically, meaning without -fPIC and the sared libcyrus_la is linked with non-PIC code, the text section is read/write, so that it cannot be shared (http://stackoverflow.com/questions/8331456/mixing-pic-and-non-pic-objects-in-a-shared-library) : all programs that dynamically link with libcyrus need their own copy of the text section from libcyrus.

in practiice, I cannot verify this, as with ld 2.27.51.20161123 and gcc 6.2.1 20161114 the code does not compile:

make V=1
make all-recursive
make[1]: Entering directory '/tmp/cyrus-imapd'
Making all in .
make[2]: Entering directory '/tmp/cyrus-imapd'
/bin/sh ./libtool --tag=CC --mode=link gcc -fvisibility=hidden -g -O2 -L/usr/local/lib -licuuc -licudata -L/usr/local/lib -lxml2 -L/usr/local/lib -lical -licalss -licalvcal -lpthread -licuuc -licui18n -L/usr/local/lib -ljansson -L/usr/local/lib -lnghttp2 -lm -o lib/libcyrus.la -rpath /usr/local/lib lib/lib_libcyrus_la-acl.lo lib/lib_libcyrus_la-acl_afs.lo lib/lib_libcyrus_la-auth.lo lib/lib_libcyrus_la-auth_krb.lo lib/lib_libcyrus_la-auth_krb5.lo lib/lib_libcyrus_la-auth_pts.lo lib/lib_libcyrus_la-auth_unix.lo lib/lib_libcyrus_la-bitvector.lo lib/lib_libcyrus_la-bsearch.lo lib/lib_libcyrus_la-byteorder64.lo lib/lib_libcyrus_la-charset.lo lib/lib_libcyrus_la-command.lo lib/lib_libcyrus_la-cyrusdb.lo lib/lib_libcyrus_la-cyrusdb_flat.lo lib/lib_libcyrus_la-cyrusdb_quotalegacy.lo lib/lib_libcyrus_la-cyrusdb_skiplist.lo lib/lib_libcyrus_la-cyrusdb_twoskip.lo lib/lib_libcyrus_la-glob.lo lib/lib_libcyrus_la-htmlchar.lo lib/lib_libcyrus_la-imapurl.lo lib/lib_libcyrus_la-imclient.lo lib/lib_libcyrus_la-imparse.lo lib/lib_libcyrus_la-iostat.lo lib/lib_libcyrus_la-iptostring.lo lib/lib_libcyrus_la-libcyr_cfg.lo lib/lib_libcyrus_la-lsort.lo lib/lib_libcyrus_la-mappedfile.lo lib/lib_libcyrus_la-mkgmtime.lo lib/lib_libcyrus_la-parseaddr.lo lib/lib_libcyrus_la-prot.lo lib/lib_libcyrus_la-ptrarray.lo lib/lib_libcyrus_la-rfc822tok.lo lib/lib_libcyrus_la-signals.lo lib/lib_libcyrus_la-stristr.lo lib/lib_libcyrus_la-times.lo lib/lib_libcyrus_la-tok.lo lib/lib_libcyrus_la-wildmat.lo lib/lib_libcyrus_la-cyrusdb_sql.lo lib/lib_libcyrus_la-sqldb.lo lib/lib_libcyrus_la-gmtoff_tm.lo lib/lib_libcyrus_la-nonblock_fcntl.lo lib/lib_libcyrus_la-chartable.lo libcrc32.a -lsasl2 -L/usr/local/lib -lssl -lcrypto -lssl -lcrypto -lgssapi_krb5 -lkrb5 -lk5crypto -lcom_err -lkrb5support -lpcre -lpcreposix -lz -L/usr/local/lib -lxml2 -L/usr/local/lib -lical -licalss -licalvcal -lpthread -licuuc -licui18n -L/usr/local/lib -licui18n -licuuc -licudata -lsqlite3

*** Warning: Linking the shared library lib/libcyrus.la against the
*** static library libcrc32.a is not portable!
libtool: link: gcc -shared -fPIC -DPIC lib/.libs/lib_libcyrus_la-acl.o lib/.libs/lib_libcyrus_la-acl_afs.o lib/.libs/lib_libcyrus_la-auth.o lib/.libs/lib_libcyrus_la-auth_krb.o lib/.libs/lib_libcyrus_la-auth_krb5.o lib/.libs/lib_libcyrus_la-auth_pts.o lib/.libs/lib_libcyrus_la-auth_unix.o lib/.libs/lib_libcyrus_la-bitvector.o lib/.libs/lib_libcyrus_la-bsearch.o lib/.libs/lib_libcyrus_la-byteorder64.o lib/.libs/lib_libcyrus_la-charset.o lib/.libs/lib_libcyrus_la-command.o lib/.libs/lib_libcyrus_la-cyrusdb.o lib/.libs/lib_libcyrus_la-cyrusdb_flat.o lib/.libs/lib_libcyrus_la-cyrusdb_quotalegacy.o lib/.libs/lib_libcyrus_la-cyrusdb_skiplist.o lib/.libs/lib_libcyrus_la-cyrusdb_twoskip.o lib/.libs/lib_libcyrus_la-glob.o lib/.libs/lib_libcyrus_la-htmlchar.o lib/.libs/lib_libcyrus_la-imapurl.o lib/.libs/lib_libcyrus_la-imclient.o lib/.libs/lib_libcyrus_la-imparse.o lib/.libs/lib_libcyrus_la-iostat.o lib/.libs/lib_libcyrus_la-iptostring.o lib/.libs/lib_libcyrus_la-libcyr_cfg.o lib/.libs/lib_libcyrus_la-lsort.o lib/.libs/lib_libcyrus_la-mappedfile.o lib/.libs/lib_libcyrus_la-mkgmtime.o lib/.libs/lib_libcyrus_la-parseaddr.o lib/.libs/lib_libcyrus_la-prot.o lib/.libs/lib_libcyrus_la-ptrarray.o lib/.libs/lib_libcyrus_la-rfc822tok.o lib/.libs/lib_libcyrus_la-signals.o lib/.libs/lib_libcyrus_la-stristr.o lib/.libs/lib_libcyrus_la-times.o lib/.libs/lib_libcyrus_la-tok.o lib/.libs/lib_libcyrus_la-wildmat.o lib/.libs/lib_libcyrus_la-cyrusdb_sql.o lib/.libs/lib_libcyrus_la-sqldb.o lib/.libs/lib_libcyrus_la-gmtoff_tm.o lib/.libs/lib_libcyrus_la-nonblock_fcntl.o lib/.libs/lib_libcyrus_la-chartable.o -L/usr/local/lib -L/lib64 /usr/local/lib/libjansson.so /usr/local/lib/libnghttp2.so libcrc32.a /usr/local/lib/libsasl2.so -lresolv -lssl -lcrypto -lgssapi_krb5 -lkrb5 -lk5crypto -lcom_err -lkrb5support /usr/local/lib/libpcreposix.so /usr/local/lib/libpcre.so /usr/local/lib/libxml2.so -lz /usr/local/lib/liblzma.so -lm -lical -licalss -licalvcal -licui18n -licuuc -licudata /usr/local/lib/libsqlite3.so -ldl -lpthread -g -O2 -pthread -Wl,-soname -Wl,libcyrus.so.0 -o lib/.libs/libcyrus.so.0.0.0
/usr/local/lib/gcc/x86_64-pc-linux-gnu/6.2.1/../../../../x86_64-pc-linux-gnu/bin/ld: libcrc32.a(libcrc32_a-crc32.o): relocation R_X86_64_32S against .rodata' can not be used when making a shared object; recompile with -fPIC /usr/local/lib/gcc/x86_64-pc-linux-gnu/6.2.1/../../../../x86_64-pc-linux-gnu/bin/ld: libcrc32.a(libcrc32_a-crc32c.o): relocation R_X86_64_32S against .rodata' can not be used when making a shared object; recompile with -fPIC
/usr/local/lib/gcc/x86_64-pc-linux-gnu/6.2.1/../../../../x86_64-pc-linux-gnu/bin/ld: final link failed: Nonrepresentable section on output
collect2: error: ld returned 1 exit status
make[2]: *** [Makefile:3044: lib/libcyrus.la] Error 1
make[2]: Leaving directory '/tmp/cyrus-imapd'
make[1]: *** [Makefile:5955: all-recursive] Error 1
make[1]: Leaving directory '/tmp/cyrus-imapd'
make: *** [Makefile:2631: all] Error 2

Switching from libcyrus_a to libcyrus_la as in 0ca4cc66df does solve the problems (warning disappears and code links with most recent ld).

Users who care about performance compile anyway everything with -O3, PGO and possibly LTO. For users who do not care about performance, the optimization flags are irrelevant. For which of those target groups is libcrc32 compiled with -O3?

Automake/Autoconf are not supposed to deal with optimization flags, it is up to the one who compiles the software to choose them, and the user shall be able to disable O3 in the same way in all software, if his compiler has known problems with O3.

You are not forced to use github for the review and github does not know what it means "related stuff".

Please add nodist_perl_libcyrus_min_la_SOURCES = $(nodist_lib_libcyrus_min_la_SOURCES) to Makefile.am on cyrus-2.5, otherwise the struct imapopt_s imapopts is unknown in perl/libcyrus_min.a and the resulting IMAP.so cannot resolve all its symbols, so cyradm does not load.

This was referenced May 8, 2017
@jsoref
Copy link
Contributor

jsoref commented May 8, 2017

I've tried to split this out as:

Hopefully splitting these should eventually enable this pr to be closed

Copy link
Contributor

@ksmurchison ksmurchison left a comment

Choose a reason for hiding this comment

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

Fixed in commit
5a22a67

Copy link
Contributor

@ksmurchison ksmurchison left a comment

Choose a reason for hiding this comment

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

Fixed in commit
238a6a4

Copy link
Contributor

@ksmurchison ksmurchison left a comment

Choose a reason for hiding this comment

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

@brong brong added the diceroll assigned by dice roll label Oct 23, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
diceroll assigned by dice roll
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants