For review - Server code clean up #744

ssorj · 2020-05-25T22:58:07Z

A cleanup of the server code.

Isolate connection and listener event handling to contexts where we can assert that either a connection or listener is in scope
Simplify the worker thread IO loop
Move the handler functions to their own file, since it's a lot of code
Move each major entity, such as connection or listener, to its own C and header file
Use consistent naming for file-private functions
Always use NULL for cleared pointers
Apply formatting using clang-format. Introduce formatting rules in the src/.clang-format file.
Use [C] for connection IDs in log messages

I'll squash this before merging.

The tests have a few failures that don't appear to be related to this change, but please double check me:

https://travis-ci.org/github/apache/qpid-dispatch/builds/690365464

There are a few questions embedded in comments here. I'd appreciate some help with those.

codecov-commenter · 2020-05-25T23:28:24Z

Codecov Report

Merging #744 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #744   +/-   ##
=======================================
  Coverage   86.99%   86.99%           
=======================================
  Files         100      100           
  Lines       21772    21772           
=======================================
  Hits        18941    18941           
  Misses       2831     2831

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 58eaa77...58eaa77. Read the comment docs.

ganeshmurthy · 2020-05-26T13:08:36Z

I pulled down this PR and tried to compile after creating a fresh build folder. I see the following

[gmurthy@localhost build]$ make install
[ 1%] Generating schema_enum.h, schema_enum.c
Scanning dependencies of target qpid-dispatch
[ 2%] Building C object src/CMakeFiles/qpid-dispatch.dir/alloc_pool.c.o
[ 3%] Building C object src/CMakeFiles/qpid-dispatch.dir/amqp.c.o
[ 4%] Building C object src/CMakeFiles/qpid-dispatch.dir/bitmask.c.o
[ 4%] Building C object src/CMakeFiles/qpid-dispatch.dir/buffer.c.o
[ 5%] Building C object src/CMakeFiles/qpid-dispatch.dir/error.c.o
[ 6%] Building C object src/CMakeFiles/qpid-dispatch.dir/connection.c.o
In file included from /usr/include/python2.7/pyconfig.h:6,
from /usr/include/python2.7/Python.h:8,
from /home/gmurthy/opensource/qpid-dispatch/include/qpid/dispatch/python_embedded.h:26,
from /home/gmurthy/opensource/qpid-dispatch/src/connection.c:25:
/usr/include/python2.7/pyconfig-64.h:1232: error: "_POSIX_C_SOURCE" redefined [-Werror]
1232 | #define _POSIX_C_SOURCE 200112L
|
In file included from /usr/include/netdb.h:25,
from /home/gmurthy/opensource/qpid-dispatch/src/connection.h:23,
from /home/gmurthy/opensource/qpid-dispatch/src/connection.c:20:
/usr/include/features.h:266: note: this is the location of the previous definition
266 | # define _POSIX_C_SOURCE 200809L
|
cc1: all warnings being treated as errors
make[2]: *** [src/CMakeFiles/qpid-dispatch.dir/build.make:166: src/CMakeFiles/qpid-dispatch.dir/connection.c.o] Error 1
make[1]: *** [CMakeFiles/Makefile2:1053: src/CMakeFiles/qpid-dispatch.dir/all] Error 2
make: *** [Makefile:161: all] Error 2
[gmurthy@localhost build]$

ganeshmurthy · 2020-05-26T13:15:02Z

It says ganeshmurthy closed this 5 minutes ago. I just added a comment but did not close this PR. Not sure how this got closed.

ganeshmurthy · 2020-05-26T13:17:37Z

It looks like accidentally pressed the "Close and comment" button instead of the "Comment". D'Oh. Sorry

ssorj · 2020-05-26T16:46:48Z

@ganeshmurthy what OS version did you try to build on?

ganeshmurthy · 2020-05-26T18:44:31Z

@ssorj My laptop is using Fedora 30

I also used this Docker environment by changing fedora:latest to fedora:30
https://github.com/apache/qpid-dispatch/blob/master/dockerfiles/Dockerfile-fedora

and I saw the same error as above

ChugR · 2020-05-26T20:30:22Z

Running Fedora 31 there's an error in libwebsockets:

[ 78%] Building C object src/CMakeFiles/qpid-dispatch.dir/http-libwebsockets.c.o
/home/chug/git/qpid-dispatch/src/http-libwebsockets.c: In function ‘handle_events’:
/home/chug/git/qpid-dispatch/src/http-libwebsockets.c:182:51: error: dereferencing pointer to incomplete type ‘qd_connection_t’ {aka ‘struct qd_connection_t’}
  182 |             if (!qd_server_handle_event(c->qd_conn->server, e)) {
      |                                                   ^~
/home/chug/git/qpid-dispatch/src/http-libwebsockets.c: In function ‘qd_http_listener_free’:
/home/chug/git/qpid-dispatch/src/http-libwebsockets.c:276:21: error: dereferencing pointer to incomplete type ‘qd_listener_t’ {aka ‘struct qd_listener_t’}
  276 |         hl->listener->http = NULL;
      |                     ^~
/home/chug/git/qpid-dispatch/src/http-libwebsockets.c:277:9: error: implicit declaration of function ‘qd_listener_decref’; did you mean ‘qd_listener_listen’? [-Werror=implicit-function-declaration]
  277 |         qd_listener_decref(hl->listener);
      |         ^~~~~~~~~~~~~~~~~~
  |             qd_listener_listen
cc1: all warnings being treated as errors
make[2]: *** [src/CMakeFiles/qpid-dispatch.dir/build.make:1089: src/CMakeFiles/qpid-dispatch.dir/http-libwebsockets.c.o] Error 1
make[1]: *** [CMakeFiles/Makefile2:1053: src/CMakeFiles/qpid-dispatch.dir/all] Error 2
make: *** [Makefile:161: all] Error 2

2020-05-26 16:23:46.009566503
get git facts
Dispatch git: branch testing @ 884eaed5
Proton git: branch master @ f6df369a0

jiridanek · 2020-05-26T22:05:43Z

Travis job on macOS here is similarly unhappy.

ssorj · 2020-05-27T12:38:21Z

@ganeshmurthy, would you give this another try? I was missing libwebsockets before. I think the includes are fixed now.

ChugR · 2020-05-27T14:52:54Z

src/connection.h

+    qd_timer_t*                    timer;  // Timer for initial setup
+    pn_connection_t*               pn_conn;
+    pn_session_t*                  pn_sessions[QD_SSN_CLASS_COUNT];
+    pn_ssl_t*                      ssl;
+    qd_listener_t*                 listener;
+    qd_connector_t*                connector;


By and large the format convention for existing code is to have the indirection operator close to the variable name and not close to the type name. Is there a reason for changing this?
Attaching the operator to the variable name helps one to reason correctly about the variable's type when multiple variables are declared on the same line.

Isn't that a pointer declaration, not an indirection operator? As in, it would be an indirection operator if I were dereferencing something, but I'm not.

I have no problem flipping it, and ultimately I'll apply whatever style we decide should go in .clang-format. I always think of it as type information that belongs with the type.

I don't have a problem with either way. If you apply the new style in .clang-format will doing "clang-format -i src/*.c" change the style throughout the code?
By that I mean, if we are changing the style here, we will have to do the same everywhere.

"clang-format -i src/*.c would change all the .c files there, but I wouldn't make applying clang format an all-or-nothing thing. I suggest using it when developing new code or doing a cleanup.

clang-format's formatting isn't perfect in every respect. I think it's a net win, personally, but there are cases where it's not doing what you might want. Pointer declarations is one example - with the pointer star aligned to the value, the structs don't look right.

Fortunately, you can apply it as needed and you can defeat the formatting if you need to with directives in comments.

Works best with either find or git. Something like this to reformat all, clang-format -i $(git ls-files -- *.c *.h) and to reformat changed files, clang-format -i $(git diff --name-only upstream/master..HEAD). Not sure if it is possible to only reformat changed sections of individual files like this.

Isn't that a pointer declaration, not an indirection operator?

It's both. int p; is an int declaration. int * p; is a pointer declaration. The difference between them is the *, the indirection operator. The operator has the same effect on declarations and on references.
It's unfortunate that the style settings don't have a way to specify the current style easily.

Yeah, very unfortunate: https://github.com/llvm/llvm-project/blob/a8f720925599f8e44366438f1ccb4b4e9d9375ae/clang/lib/Format/WhitespaceManager.cpp#L641-L650

ChugR · 2020-05-27T15:19:15Z

Please don't rebase and do a forced push. This messes the branches I've created on your last testing branch. https://www.atlassian.com/git/tutorials/merging-vs-rebasing#the-golden-rule-of-rebasing
It's really cheap to open a new pull request with the testing branch based at the new master node instead.

ssorj · 2020-05-27T15:47:59Z

@ChugR okay, gotcha. (I didn't anticipate anyone basing anything on this branch until it was approved.)

ganeshmurthy · 2020-05-27T18:21:51Z

@ganeshmurthy, would you give this another try? I was missing libwebsockets before. I think the includes are fixed now.

This looks good now. No compile errors. Thanks.

ted-ross · 2020-05-28T01:40:54Z

Re: pointer declarations:

Please do this the way Chuck suggests:

qd_timer_t   *timer;
int           count;

This is the convention used throughout the code.

Thanks,
-Ted

gemmellr · 2020-05-28T08:50:47Z

On the 'dont force push to your PR branch' bit..

On the flipside, for something like this I far prefer updating the single PR (even with force pushes) than opening lots of new PRs and spreading related discussion over a bunch of them.

If that discussion starts off saying 'this is just wrong, lets do some other entirely different thing, fix the actual problem, rather than whatever this does', then perhaps a new PR might be more appropriate.

Essentially, I think of PR branches in a repo fork as all but private unless specifically agreed otherwise with given collaborators, and basing new branches on such known-PR sources from the fork is to accept that you might need to fix up your branches when they change and thats ok (I did that yesterday for $reasons, I had to rebase, that was fine). I wouldn't by default consider them the same thing as shared repo public feature branches like the Atlassian page discusses, which is more along the lines of someone made a feature branch in the lone shared public repo where everyone works.

ssorj · 2020-05-28T13:31:03Z

@gemmellr I asked the folks on the Proton team, and it's also conventional there to rebase PR branches.

I'm totally willing to put something like "-wip" in my branch name as a way of saying "don't branch off of this unless you accept the consequences".

gemmellr · 2020-05-28T13:50:06Z

I dont think that is needed personally, its fairly common practice for PR branches unless agreed otherwise. GitHub even allows for maintainers to update your PR on your behalf (if you've allowed it), and I have had such force pushes done on PRs with it enabled.

jiridanek · 2020-05-30T15:18:54Z

docs/notes/code-conventions.md

+<!-- specific language governing permissions and limitations -->
+<!-- under the License. -->
+
+# Code conventions


Other documentation is formatted in AsciiDoc. It would be better not to introduce Markdown in addition.

jiridanek · 2020-05-30T15:28:31Z

src/connection.c

+                semi_colon_count++;
+            } else {
+                // This is an unrecognized component.  Log a critical
+                // error.


clang-format did not reflow this one. It will add breaks if a comment is over column limit, but it apparently does not try to join lines. Makes sense, joining lines can easily break meaningful formatting.

jiridanek · 2020-05-30T15:35:55Z

src/server.c

+// Called with qd_python_lock held.
+//
+// XXX But it doesn't use the _lh convention?
+qd_error_t qd_register_display_name_service(qd_dispatch_t* qd, void* displaynameservice) {


Previously the doc blocks were formatted as doxygen comment blocks, while the new comments aren't. Is that a plan?

We've been pretty lax about the format of function documentation. While I like the way the doxygen format reads in the code, I've never actually generated (or used) documentation from the source.

Way back in the beginning we designed a big chunk of what is now the router as a library intended to be public. We no longer have that goal, but it may explain why some functions have more formal documentation than others.

jiridanek · 2020-05-30T15:43:11Z

src/server.c


+    // XXX When is config going to be null?
+    if (config && config->inter_router_cost > 1) {


Btw, this looks to me like a response to a Coverity warning. Coverity likes to send reports about NULL config ;P The Coverity results on the page usually have something they call "static trace", which is trying to answer this question...

Agreed - if coverity ever encounters a path through the code where a pointer is checked for null, then it flags any use of that pointer that isn't checked for null as a possible error.

jiridanek · 2020-05-30T15:44:35Z

src/server.c

+    server->sasl_config_name     = sasl_config_name;
+    server->proactor             = pn_proactor();
+    server->container            = NULL;
+    server->start_context        = 0;  // XXX Should be NULL?


Probably, since this is typed as a void pointer.

In general the code avoids using NULL in favor of 0. Not arguing for or against - that's Just the Way It Is.
Probably should be consistent.

In C++, clang-tidy pushes towards using the (cpp only) nullptr. Not sure what cppcheck does. I guess that C linters, if any was used, would, by default, push for using NULL.

jiridanek · 2020-05-30T15:47:24Z

src/connection_manager.c

@@ -157,6 +160,7 @@ static qd_config_sasl_plugin_t *qd_find_sasl_plugin(qd_connection_manager_t *cm,
    return 0;
 }

+// XXX Should this move to server.c or its own server_config.c?
 void qd_server_config_free(qd_server_config_t *cf)
 {


Btw, I hate having to create new files. I always feel like I am polluting someone's else carefully thought out layout...

Bbtw, I thought that clang-format with the style selected would move all opening braces on the same line. Apparently not.

kgiusti

I've left a few style-related comments inline.

From a more structural point of view I'm a big fan of breaking stuff up into separate source files that contain one primary public object. And a corresponding header for the object's API that lives in the top level include/qpid/dispatch directory.

We also have a convention for when a subset of an objects' API is considered internal but needs to be shared across sources. This portion of the API is put into a separate header file named object_private.h that is located in the source directory at the same level as the corresponding object.c file.

So in general I agree with the way this patch breaks things up.

kgiusti · 2020-06-05T14:21:34Z

src/connection_manager.c

@@ -157,6 +160,7 @@ static qd_config_sasl_plugin_t *qd_find_sasl_plugin(qd_connection_manager_t *cm,
    return 0;
 }

+// XXX Should this move to server.c or its own server_config.c?


IIRC the qd_server_config_t structure is public - connection_manager and server (perhaps others) have direct access to its fields. I prefer breaking these out to their own .h and .c files.

kgiusti · 2020-06-05T14:26:36Z

src/connector.c

+
+ALLOC_DEFINE(qd_connector_t);
+
+const char* qd_connector_policy_vhost(qd_connector_t* connector) {


Our code convention puts the opening brace for functions on its own line directly under the function. All other C constructs - structs, enums, control statements - put the opening brace on the same line.

Closing braces always appear on their own line.

kgiusti · 2020-06-05T14:34:33Z

src/server.c

+// Called with qd_python_lock held.
+//
+// XXX But it doesn't use the _lh convention?
+qd_error_t qd_register_display_name_service(qd_dispatch_t* qd, void* displaynameservice) {


We've been pretty lax about the format of function documentation. While I like the way the doxygen format reads in the code, I've never actually generated (or used) documentation from the source.

Way back in the beginning we designed a big chunk of what is now the router as a library intended to be public. We no longer have that goal, but it may explain why some functions have more formal documentation than others.

kgiusti · 2020-06-05T14:36:32Z

src/server.c

    if (displaynameservice) {
        qd->server->py_displayname_obj = displaynameservice;
-        Py_XINCREF((PyObject *)qd->server->py_displayname_obj);
+        Py_XINCREF((PyObject*) qd->server->py_displayname_obj);


IIUC the (unspoken) convention is to put a space between and * in casts.
A space between the closing paran on the cast and the casted object should be part of the convention going forward.

kgiusti · 2020-06-05T14:38:41Z

src/server.c

+    server->sasl_config_name     = sasl_config_name;
+    server->proactor             = pn_proactor();
+    server->container            = NULL;
+    server->start_context        = 0;  // XXX Should be NULL?


In general the code avoids using NULL in favor of 0. Not arguing for or against - that's Just the Way It Is.
Probably should be consistent.

kgiusti · 2020-06-05T14:42:47Z

src/server.c


+    // XXX When is config going to be null?
+    if (config && config->inter_router_cost > 1) {


Agreed - if coverity ever encounters a path through the code where a pointer is checked for null, then it flags any use of that pointer that isn't checked for null as a possible error.

…ection

…g-based formatting

jiridanek · 2020-10-09T16:26:23Z

Thinking back to this, my take regarding reasons why this was not merged was 1} disagreement with the code formatting change (llvm formatter does not support well the current style used in dispatch), and possibly 2} that it will break outstanding work in progress.

The moving of things around in this PR was actually received well, as far as I can tell from the comments.

jiridanek · 2021-02-01T13:59:51Z

Re: pointer declarations:

Please do this the way Chuck suggests:
qd_timer_t   *timer;
int           count;
This is the convention used throughout the code.

Thanks,
-Ted

@mgoulish just volunteered to reformat Dispatch to something more palatable to contemporary tooling (clang-format). Thanks, Mick, much appreciated!

…00 bec… (apache#744) * Fixes apache#637: Reduce the number of links created from 10000 to 4000 because the test is exceeding 400 seconds in CI * Python checker fixes

ssorj requested review from kgiusti and ganeshmurthy May 25, 2020 22:58

ganeshmurthy closed this May 26, 2020

ganeshmurthy reopened this May 26, 2020

ssorj force-pushed the testing branch from 69d430a to a81204d Compare May 27, 2020 11:41

ChugR reviewed May 27, 2020

View reviewed changes

jiridanek reviewed May 30, 2020

View reviewed changes

kgiusti reviewed Jun 5, 2020

View reviewed changes

Cleanup

31c933c

ssorj added 24 commits August 10, 2020 07:21

Format tweaks

d6c45a2

WIP

a1ae29d

Use the standard entrypoint for event handling in the websocket code

61d1d89

Move auth service connection handling above the assertion for qd_conn…

53beb6b

…ection

WIP

07ec210

Update test for a logging change

189eab9

Use the right logger

499aa93

The run loop that I want

b171331

Formatting

9e3e42f

Various cleanups

3e967a0

Restore post-batch free list processing

05ab829

WIP

4ccf0bb

Fix server_thread_run, and more cleanup

5a4bb39

Formatting

619257a

Remove an obsolete comment

7635190

Make structs look nicer

036ec41

Add a license header

8084144

Remove alternate server_thread_run impl

de8a368

Qualify the names of the doc developer notes

cf5c7c9

Convert code conventions to a markdown file and add a section on Clan…

953847e

…g-based formatting

Fix some includes

3cc04f2

Some style adjustments

a67dbba

A WIP struct formatter

f851f7f

Restore the open properties handling

593fb0e

ssorj force-pushed the testing branch from a81204d to 593fb0e Compare August 10, 2020 12:17

ganeshmurthy closed this Jan 19, 2022

jiridanek mentioned this pull request May 26, 2022

Initial clang-format work: config file and git-clang-format instructions skupperproject/skupper-router#504

Merged

4 tasks


		// XXX When is config going to be null?
		if (config && config->inter_router_cost > 1) {


		ALLOC_DEFINE(qd_connector_t);

		const char* qd_connector_policy_vhost(qd_connector_t* connector) {

For review - Server code clean up #744

For review - Server code clean up #744

Conversation

ssorj commented May 25, 2020

codecov-commenter commented May 25, 2020

Codecov Report

ganeshmurthy commented May 26, 2020 • edited

ganeshmurthy commented May 26, 2020

ganeshmurthy commented May 26, 2020

ssorj commented May 26, 2020

ganeshmurthy commented May 26, 2020

ChugR commented May 26, 2020

jiridanek commented May 26, 2020

ssorj commented May 27, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChugR commented May 27, 2020

ssorj commented May 27, 2020

ganeshmurthy commented May 27, 2020

ted-ross commented May 28, 2020

gemmellr commented May 28, 2020 • edited

ssorj commented May 28, 2020

gemmellr commented May 28, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jiridanek May 30, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jiridanek May 30, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kgiusti left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jiridanek commented Oct 9, 2020

jiridanek commented Feb 1, 2021

ganeshmurthy commented May 26, 2020 •

edited

gemmellr commented May 28, 2020 •

edited

gemmellr commented May 28, 2020 •

edited

jiridanek May 30, 2020 •

edited

jiridanek May 30, 2020 •

edited

kgiusti left a comment •

edited