Log dependencies #174

nmathewson · 2018-06-22T16:18:20Z

No description provided.

Minimizing includes revealed other places includes were necessary.

Containers were using crypto_digest.h, just to see the value of DIGEST_LEN. Moved those constants into a new defs module.

The other time stuff is higher-level

Now that I know that "strings" nests below "container", I know this is safe.

The locking code gets its own module, since it's more fundamental than the higher-level locking code. Extracting the logging code was the whole point here. :)

This change makes a whole bunch of things in torlog.c break, since apparently I did not find all the fd dependencies.

This patch: - introduces an fdio module for low-level fd functions that don't need to log. - moves the responsibility for opening files outside of torlog.c, so it won't need to call tor_open_cloexec.

coveralls · 2018-06-22T18:34:02Z

Coverage decreased (-0.03%) to 59.862% when pulling 1b93b06 on nmathewson:log_dependencies into eb784aa on torproject:master.

teor2345 · 2018-06-25T04:28:44Z

What's the trac ticket here?

dgoulet-tor · 2018-06-26T15:22:55Z

src/common/util.h

@@ -23,6 +23,7 @@
 #include <sys/stat.h>
 #endif
 #include "lib/err/torerr.h"
+#include "lib/malloc/util_malloc.h"


I'll bikeshed here but I think util.h or even malloc.h (I might personally prefer util.h) could be better here because we are already in the "malloc module" so util_malloc.h when we enforce the lib/malloc/ include statement is redundant.

I think malloc.h is fine, but not util.h -- that would mean we had a bunch of files named util.h, and I don't want to have two headers or c files with the same name.

dgoulet-tor · 2018-06-26T15:22:55Z

src/lib/container/bloomfilt.c

+ * with helper functions to use smartlists.  Also includes
+ * hash table implementations of a string-to-void* map, and of
+ * a digest-to-void* map.
+ **/


This whole section is wrong.

dgoulet-tor · 2018-06-26T15:22:55Z

src/lib/container/bloomfilt.h

+  int mask; /**< One less than the number of bits in <b>ba</b>; always one less
+             * than a power of two. */
+  bitarray_t *ba; /**< A bit array to implement the Bloom filter. */
+} digestset_t;


I think, this can be done after, we should namespace these with the bloomfilt name even thought "digest set" is technically valid.

dgoulet-tor · 2018-06-26T15:22:55Z

src/lib/container/order.c

+ * with helper functions to use smartlists.  Also includes
+ * hash table implementations of a string-to-void* map, and of
+ * a digest-to-void* map.
+ **/


Bad comments, I guess copy pasting :). Seems many files in lib/container/ suffer from this.

dgoulet-tor · 2018-06-26T15:22:55Z

src/include.am

@@ -5,6 +5,7 @@ include src/lib/ctime/include.am
 include src/lib/compress/include.am
 include src/lib/container/include.am
 include src/lib/crypt_ops/include.am
+include src/lib/defs/include.am


What does defs/ stands for here? "Definitions" I presume? Would we put all global constant in there or ?...

yes, definitions. But I think only top-level definitions that need to be really global.

dgoulet-tor · 2018-06-26T15:22:55Z

src/lib/string/include.am

+
+src_lib_libtor_string_a_SOURCES =			\
+	src/lib/string/compat_ctype.c			\
+	src/lib/string/util_string.c			\


I might have the same comment as malloc module here with this util_string.c.

dgoulet-tor · 2018-06-26T15:22:55Z

.gitignore

@@ -173,6 +173,8 @@ uptime-*.json
 /src/lib/libtor-ctime-testing.a
 /src/lib/libtor-err.a
 /src/lib/libtor-err-testing.a
+/src/lib/libtor-intmath.a


What about a math module instead and we put everything related to "math" in there (not only int stuff). I mean with this name, does the floating math functions apply to go in?

no, higher-level stuff should go in another module.

This one is for low-level integer math.

dgoulet-tor · 2018-06-26T15:22:55Z

src/lib/wallclock/include.am

@@ -7,6 +7,7 @@ endif

 src_lib_libtor_wallclock_a_SOURCES =			\
 	src/lib/wallclock/approx_time.c			\
+	src/lib/wallclock/tm_cvt.c			\


I'm not familiar with this naming, what does tm_cvt means here?

tm is "struct tm"; cvt is convert.

dgoulet-tor · 2018-06-26T15:26:29Z

Makefile.am

@@ -42,6 +42,7 @@ TOR_UTIL_LIBS = \
 	src/common/libor.a \
        src/lib/libtor-log.a \
        src/lib/libtor-lock.a \
+        src/lib/libtor-fdio.a \


Quick question: Is the goal of this library to also get the tor_open_* family of functions (currently mostly in compat.c)?

If yes, I'm wondering if libtor-io.a wouldn't be better and we could also bring in all the network low level functions? Or for that you had in mind a libtor-network. kind of separation?

No, tor_open_* needs to be at a higher level, or else it will cause a circular dependency. Those functions have a lot of complexity, and have the potential to log, and require the sandbox code.

nmathewson added 30 commits Jun 21, 2018

Extract tor_malloc and friends to a new module.

e066966

Simplify include structure of container.[ch]

f95e3bf

Refactor container into a library.

77dff00

Rectify include paths after container split (automatic)

b8be826

Split container.c based on container types, and minimize includes

657ff55

Minimizing includes revealed other places includes were necessary.

Move STRUCT_VAR_P to compat_compiler.

479c2ab

Remove container->crypto dependency

932b4d0

Containers were using crypto_digest.h, just to see the value of DIGEST_LEN. Moved those constants into a new defs module.

Remove bloom filters, order statistics, and bitarrays from container.h

50a5954

Remove map from container.h

9cf6fc9

Extract smartlist.h from container.h

de508c5

Expunge container.h

c2a558a

Update permissible includes

d1cada5

Extract our code for answering "what time is it right now".

9426751

The other time stuff is higher-level

Remove compat.h as unneeded from log.c and torlog.h

0932f32

Move ARRAY_LENGTH to compiler_compat.h

1e07b40

Extract key string manipulation functions into a new library.

1abadee

Move smartlist_add_{v,}asprintf into smartlist.[ch]

3883338

Now that I know that "strings" nests below "container", I know this is safe.

Extract simple integer math into its own module

2cf033f

Extract the locking and logging code

97b15a1

The locking code gets its own module, since it's more fundamental than the higher-level locking code. Extracting the logging code was the whole point here. :)

Automated fixup of include paths after torlog.h movement.

da4ae8a

Remove the util_bug.h include from smartlist.h.

b2d4e78

This change makes a whole bunch of things in torlog.c break, since apparently I did not find all the fd dependencies.

Extract core part of gmtime_r, localtime_r (without logging)

bfb3916

Extract strlcpy and strlcmp to libtor-string

90a09df

Finally extract the log library and make it build.

79f73ab

This patch: - introduces an fdio module for low-level fd functions that don't need to log. - moves the responsibility for opening files outside of torlog.c, so it won't need to call tor_open_cloexec.

Update .gitignore and .may_include files

7a93ce8

Remove util_bug dependency on compat.h

6fc2d53

Move util_bug into libtor-log

bcf3e54

Fix up the rust build script library list.

b0adf2f

A pair of missing includes.

7aecea7

Two more small changes for CI.

cf66544

nmathewson added 6 commits Jun 22, 2018

Update the micro-revision.i dependencies, and add a stdlib.h

4118ba6

Remove an "m" that did not belong.

76a7178

Add another include for windows, and change a log to a raw_assert

145665a

We also need torerr in tm_cvt.

4b32446

Another windows include

405fa42

Make an inline static so we can build with coverage enabled.

1b93b06

dgoulet-tor reviewed Jun 26, 2018

View changes

tor-bot merged commit 1b93b06 into torproject:master Jun 26, 2018
2 of 3 checks passed

torproject / tor Public

Log dependencies #174

Log dependencies #174

nmathewson commented Jun 22, 2018

coveralls commented Jun 22, 2018

teor2345 commented Jun 25, 2018

dgoulet-tor Jun 26, 2018

nmathewson Jun 26, 2018

dgoulet-tor Jun 26, 2018

dgoulet-tor Jun 26, 2018

dgoulet-tor Jun 26, 2018

dgoulet-tor Jun 26, 2018

nmathewson Jun 26, 2018

dgoulet-tor Jun 26, 2018

dgoulet-tor Jun 26, 2018

nmathewson Jun 26, 2018

dgoulet-tor Jun 26, 2018

nmathewson Jun 26, 2018

dgoulet-tor Jun 26, 2018

nmathewson Jun 26, 2018

torproject / tor Public

Log dependencies #174

Log dependencies #174

Conversation

nmathewson commented Jun 22, 2018

coveralls commented Jun 22, 2018

teor2345 commented Jun 25, 2018