libflux: clean up bulk message handler registration functions #1277

garlick · 2017-11-07T23:29:26Z

This is following up on an idea @morrone had in #1135 to clean up the bulk message handler reg/unreg functions. The flux_msg_handler_t member is dropped from struct flux_msg_handler_spec, and instead a new array of message handlers is returned from flux_msg_handler_addvec().

flux_msg_handler_delvec() then accepts the array of message handlers, destroying them and freeing the array.

In addition, a small mention about rolemask security is added to the message handler man pages.

Updated users, tests, and man pages.

This will require a PR to flux-sched. Please don't merge until that's queued up.

coveralls · 2017-11-08T00:09:14Z

Coverage increased (+0.03%) to 78.541% when pulling ec12939 on garlick:msg_handlers into 4456339 on flux-framework:master.

codecov-io · 2017-11-08T00:09:31Z

Codecov Report

Merging #1277 into master will increase coverage by 0.04%.
The diff coverage is 80.19%.

@@            Coverage Diff             @@
##           master    #1277      +/-   ##
==========================================
+ Coverage   77.92%   77.97%   +0.04%     
==========================================
  Files         154      154              
  Lines       29084    29104      +20     
==========================================
+ Hits        22665    22693      +28     
+ Misses       6419     6411       -8

Impacted Files	Coverage Δ
src/broker/hello.c	`78.89% <100%> (+0.19%)`	⬆️
src/broker/log.c	`72.89% <100%> (ø)`	⬆️
src/broker/content-cache.c	`73.43% <100%> (ø)`	⬆️
src/broker/shutdown.c	`89.33% <100%> (+0.14%)`	⬆️
src/broker/heaptrace.c	`89.28% <100%> (ø)`	⬆️
src/broker/broker.c	`72.33% <100%> (-0.02%)`	⬇️
src/broker/exec.c	`70.38% <100%> (ø)`	⬆️
src/broker/attr.c	`85.78% <100%> (-0.14%)`	⬇️
src/cmd/builtin/proxy.c	`75.19% <100%> (+0.04%)`	⬆️
src/broker/sequence.c	`94.31% <100%> (+2.39%)`	⬆️
... and 27 more

chu11

Nothing caught my eye, looks pretty good to me.

Just to double check, a flux-sched fixup is also needed?

garlick · 2017-11-08T16:48:56Z

Yes, I'll post a sched PR once @morrone chimes in here.

coveralls · 2017-11-08T17:14:07Z

Coverage increased (+0.04%) to 78.592% when pulling 51dc19f on garlick:msg_handlers into b10c1c7 on flux-framework:master.

grondo · 2017-11-08T17:31:07Z

Looks good to me as well. I would have leaned toward renaming the functions to something more like create/destroy, but after going through the code I think addvec/delvec works just as well so probably a better choice to keep the existing naming.

garlick · 2017-11-08T18:17:39Z

Thanks disembodied voice of @grondo from vacation :-)

coveralls · 2017-11-08T21:22:26Z

Coverage increased (+0.04%) to 78.57% when pulling d0ea037 on garlick:msg_handlers into cc5cd0a on flux-framework:master.

coveralls · 2017-11-09T20:01:16Z

Coverage increased (+0.02%) to 78.577% when pulling 73a65bd on garlick:msg_handlers into b989b36 on flux-framework:master.

coveralls · 2017-11-10T02:40:39Z

Coverage increased (+0.04%) to 78.568% when pulling 4497b47 on garlick:msg_handlers into 819d8c5 on flux-framework:master.

morrone

Mainly, I would like to see the "cleanup addvec/delvec" commit reworked to only deal with the logical changes to separate the flux_msg_handler_t * member from flux_msg_handler_spec. Other logical changes should be in other commits.

There are some other smaller comments inline.

morrone · 2017-11-10T18:13:27Z

src/broker/attr.c

-    { FLUX_MSGTYPE_REQUEST, "attr.set",    setattr_request_cb, 0, NULL },
+static const struct flux_msg_handler_spec handlers[] = {
+    { FLUX_MSGTYPE_REQUEST, "attr.get",    getattr_request_cb, FLUX_ROLE_ALL },
+    { FLUX_MSGTYPE_REQUEST, "attr.list",   lsattr_request_cb, FLUX_ROLE_ALL },


This change from 0 (FLUX_ROLE_NONE) to FLUX_ROLE_ALL looks like something that should be broken out into a separate commit.

That's a hard one to split out. The code was setting the rolemask to 0, then changing it with code like

flux_msg_handler_allow_rolemask (handlers[0].w, FLUX_ROLE_ALL);

Since w is gone, I thought it best to change the initial setting rather than walk the handlers list or whatever.

I'm confused about why that is hard. Doesn't

flux_msg_handler_allow_rolemask (handlers[0].w, FLUX_ROLE_ALL);

just change into

flux_msg_handler_allow_rolemask (attrs->handlers[0], FLUX_ROLE_ALL);

No walking seems to be required.

But again, I perfectly fine with changing the place the rolemask is set. You could even make that rolemask location change in a commit before this one. It is just a change that isn't really pertinent to this commit.

This is minor enough though that I am willing to let it slide.

morrone · 2017-11-10T18:29:34Z

src/broker/broker.c

@@ -668,6 +669,8 @@ int main (int argc, char *argv[])
    service_switch_destroy (ctx.services);
    hello_destroy (ctx.hello);
    attr_destroy (ctx.attrs);
+    shutdown_destroy (ctx.shutdown);


This looks like a good improvement, but unrelated to this commit's logical topic.

Agreed, will split out.

morrone · 2017-11-10T18:33:08Z

src/broker/broker.c

@@ -668,6 +669,8 @@ int main (int argc, char *argv[])
    service_switch_destroy (ctx.services);
    hello_destroy (ctx.hello);
    attr_destroy (ctx.attrs);
+    shutdown_destroy (ctx.shutdown);
+    flux_msg_handler_delvec (ctx.handlers);


This looks good, but I'm not a fan of the lack of symmetry. Since the matching flux_msg_handler_addvec() is hidden down one call level in broker_add_services(), I think it would be nice to have a matching broker_remove_services() where flux_msg_handler_delvec() is called.

Good suggestion.

morrone · 2017-11-10T18:42:10Z

src/broker/heaptrace.c

@@ -38,6 +38,8 @@
 #include <flux/core.h>
 #include "heaptrace.h"

+static flux_msg_handler_t **handlers = NULL;


In C, file scope variables are already initialized to 0 so the "= NULL" is redundant. Not a big deal.

morrone · 2017-11-10T19:03:05Z

src/common/libflux/msg_handler.c

    return -1;
 }

-void flux_msg_handler_delvec (struct flux_msg_handler_spec tab[])
+void flux_msg_handler_delvec (flux_msg_handler_t **handlers)


*handlers[]?

morrone · 2017-11-10T19:16:18Z

src/common/libflux/msg_handler.c

-            break; /* FLUX_MSGHANDLER_TABLE_END */
+    if (!h || !tab || !hp) {
+        errno = EINVAL;
+        goto error;


There would appear to be no reason to call flux_msg_handler_delvec() if this memory allocation fails. This should probably jump to another label just before "return -1".

morrone · 2017-11-10T19:16:51Z

src/common/libflux/msg_handler.c

+    while (!at_end (tab[count]))
+        count++;
+    if (!(handlers = calloc (count + 1, sizeof (flux_msg_handler_t *))))
+        goto error;


This shouldn't call flux_msg_handler_delvec() on error either.

morrone · 2017-11-10T19:18:55Z

src/common/libflux/msg_handler.c

-            tab[i].w = NULL;
+    if (handlers) {
+        int i;
+        for (i = 0; handlers[i] != NULL; i++) {


Since we are using C99, I would prefer to see this be "for (int i =0;" rather than declare i on its own line.

morrone · 2017-11-10T19:28:41Z

src/common/libjsc/jstatctl.c

@@ -1144,19 +1147,19 @@ static int notify_status_obj (flux_t *h, jsc_handler_obj_f func, void *d)
        rc = -1;
        goto done;
    }
-    if (flux_msg_handler_addvec (h, htab, (void *)ctx) < 0) {
+    if (flux_msg_handler_addvec (h, htab, NULL, &handlers) < 0) {


The elimination of (void *)ctx doesn't appear to be related to this logical commit.

Yes, will split out.

morrone · 2017-11-10T19:34:36Z

src/modules/wreck/job.c

        flux_log_error (h, "flux_msg_handler_addvec");
-        return (-1);
+        goto done;


This jumps to the wrong place. If flux_msg_handler_addvec() failed, we shouldn't be calling flux_msg_handler_delvec().

delvec is a no-op so it's not wrong per se. Sometimes it's nice to have a single exit path rather than returns sprinkled throughout a function. I could go either way here but I'll do as you suggest.

garlick · 2017-11-10T22:16:32Z

Mainly, I would like to see the "cleanup addvec/delvec" commit reworked to only deal with the logical changes to separate the flux_msg_handler_t * member from flux_msg_handler_spec. Other logical changes should be in other commits.

I can't separate the libflux/msg_handler changes from the rest without breaking git bisect, so unfortunately it's a long commit, but it'll be a bit more focused after I split out the other bits you've suggested.

Thanks for the detailed feedback!

morrone · 2017-11-10T22:54:52Z

I can't separate the libflux/msg_handler changes from the rest without breaking git bisect

I'm not entirely sure what you think I was asking for, but I don't think it was that. :) I was viewing "separate the flux_msg_handler_t * member from flux_msg_handler_spec" as the one big change in that commit. There was no additional implication meant to "separate the libflux/msg_handler changes from the rest".

garlick · 2017-11-10T23:13:45Z

Just pushed an update with the suggested changes.

coveralls · 2017-11-10T23:32:34Z

Coverage increased (+0.02%) to 78.552% when pulling ec30bff on garlick:msg_handlers into 819d8c5 on flux-framework:master.

Track API changes proposed in flux-framework/flux-core#1277, namely the definition of struct flux_msg_handler_spec and the function prototypes for flux_msg_handler_addvec() and flux_msg_handler_delvec().

morrone · 2017-11-11T00:20:59Z

src/broker/broker.c

@@ -158,7 +159,7 @@ static void broker_handle_signals (broker_ctx_t *ctx, zlist_t *sigwatchers);
 static void broker_unhandle_signals (zlist_t *sigwatchers);

 static void broker_add_services (broker_ctx_t *ctx);
-static void broker_remove_services (void);
+static void broker_remove_services (flux_msg_handler_t *handlers[]);


Thanks, that is a nice improvement! But there is still a symmetry problem here. broker_add_services takes a single "ctx" and returns nothing. The caller basically knows nothing about what happened. But then to use broker_remove_services(), somehow we need to know to give it "handlers". What handlers? How do we know that we have to dive into "ctx" to provide the correct handlers? From a code reviewer and developer's perspective, the only way to know how this works is to read all of the code involved. That tells us the interface can be improved still.

A simple solution would be to make broker_remove_services look like this:

static void broker_remove_services (broker_ctx_t *ctx);

But I think we can do better still. How about making the prototypes this:

static flux_msg_handler_t **broker_add_services (broker_ctx_t *ctx); static void broker_remove_services (flux_msg_handler_t *handlers[]);

The add function simply returns the pointer to the handlers so the caller can pass it to the remove function. This is even better because "flux_msg_handler_t **handlers;" can be defined in main() rather than in the globalish "ctx". It is always good to keep variables as local as possible.

That's reasonable.

I'm confused about why that is hard

Oh duh, not sure what I was thinking there. You are right.

morrone · 2017-11-11T00:33:45Z

src/broker/attr.c

-    { FLUX_MSGTYPE_REQUEST, "attr.set",    setattr_request_cb, 0, NULL },
+static const struct flux_msg_handler_spec handlers[] = {
+    { FLUX_MSGTYPE_REQUEST, "attr.get",    getattr_request_cb, FLUX_ROLE_ALL },
+    { FLUX_MSGTYPE_REQUEST, "attr.list",   lsattr_request_cb, FLUX_ROLE_ALL },


I'm confused about why that is hard. Doesn't

flux_msg_handler_allow_rolemask (handlers[0].w, FLUX_ROLE_ALL);

just change into

flux_msg_handler_allow_rolemask (attrs->handlers[0], FLUX_ROLE_ALL);

No walking seems to be required.

But again, I perfectly fine with changing the place the rolemask is set. You could even make that rolemask location change in a commit before this one. It is just a change that isn't really pertinent to this commit.

This is minor enough though that I am willing to let it slide.

morrone · 2017-11-11T01:04:44Z

doc/man3/flux_msg_handler_addvec.adoc

+
+The _rolemask_ member of _struct flux_msg_handler_spec_ is passed
+to `flux_msg_handler_allow_rolemask()`.  If only FLUX_ROLE_OWNER should
+be allowed to send messages to the handler, set _rolemask_ to zero.


I find this very confusing. FLUX_ROLE_NONE is 0, and FLUX_ROLE_OWNER is 1. So essentially you are saying that if we want to allow FLUX_ROLE_OWNER, we should set FLUX_ROLE_NONE.

I'm not a fan of using zero in the code when we have explicitly set a name for that, FLUX_ROLE_NONE. So at the least, I would prefer to see "zero" changed to "FLUX_ROLE_NONE".

I read the flux_msg_handler_create(3) man page as instructed below, and it says:

"A message handler only receives a message if a bit-wise AND of the message
rolemask and the message handler rolemask evaluates to a non-zero value.
By default, the message handler rolemask contains only FLUX_ROLE_OWNER
(the instance owner) but may be changed with
flux_msg_handler_allow_rolemask() and flux_msg_handler_deny_rolemask(),
which add or remove roles from the message handler rolemask. FLUX_ROLE_OWNER
cannot be dropped from the message handler rolemask."

It is pretty unclear about what happens when we try to set the role to FLUX_ROLE_NONE, despite that being exactly what is instructed in this man page. I'm assuming that it means that FLUX_ROLE_NONE is ignored and the default value of FLUX_ROLE_OWNER remains.

As I can tell from the documentation, at current time there is no practical difference between FLUX_ROLE_NONE and FLUX_ROLE_OWNER. But perhaps in the future there will be? (Otherwise, why have them both?) So perhaps better than setting it to 0/FLUX_ROLE_NONE, it would be better to instruct people to set FLUX_ROLE_OWNER if they want FLUX_ROLE_OWNER.

I'm pondering this one.

In general, I don't see a problem with 0 as a value for a bitmask as opposed to a symbolic value. FLUX_ROLE_NONE was not really intended to be used here; it was defined to be used as the initial value for the rolemask field in a message, e.g. from RFC 11

FLUX_ROLE_NONE (0) SHALL indicate "invalid rolemask".

Possibly that was a poor naming choice.

I think you are right that the way FLUX_ROLE_OWNER as described is confusing. It cannot be denied and is meaningless to allow, so perhaps instead of being described as implicitly set in the "allow mask" of a handler, it should be an error to include it in the mask or to attempt to allow/deny it. It would be unfortunate to give the impression that the owner can be denied, since the fact that the owner is all powerful is a fundamental part of the security design.

In any case this pr is about something else so maybe the best thing is to drop the man page update from this PR and do another.

Problem: man page has out of date definition of struct flux_msg_handler_spec. Add "const" to topic_glob definition.

Problem: the broker shutdown class did not properly free its resources. Call shutdown_destroy() from the broker, and add a missing call to shutdown_disarm() within shutdown_destroy(), frees frees resources associated with the shutdown timer.

Problem: broker exits without freeing message handlers associated with internal services. Add broker_remove_service() to do this, and call it from the broker exit path.

Problem: use of struct flux_msg_handler_spec for both addvec and delvec functions is confusing. Drop flux_msg_handler_t * member from struct flux_msg_handler_spec. In flux_msg_handler_addvec(), make 'tab' parameter const (purely input), and add an output parameter which is a NULL terminated list of flux_msg_handler_t's. The list of message handlers becomes the sole input to flux_msg_handler_delvec. Update users and tests. Fixes flux-framework#1135.

Problem: flux_msg_handler_addvec() is called with the 'arg' parameter set to (void *)ctx, but ctx is not yet initialized and is not used in callbacks. Set the 'arg' parameter to NULL to improve clarity.

Update function prototypes and descriptions to reflect removal of 'w' from struct flux_msg_handler_spec, and addition of 'handlers' parameter to flux_msg_handler_addvec() and replacement of 'htab' with 'handlers' in flux_msg_handler_delvec().

garlick · 2017-11-11T14:57:23Z

security documentation dropped for another PR, and requested change to broker_add_services() added.

coveralls · 2017-11-11T15:38:12Z

Coverage increased (+0.04%) to 78.566% when pulling 08cd966 on garlick:msg_handlers into 819d8c5 on flux-framework:master.

morrone · 2017-11-12T20:53:34Z

Looks pretty good. Just missing the part in broker.c about keeping the "handlers" variable local to main() rather than more globally in ctx where it is not needed.

coveralls · 2017-11-12T21:26:50Z

Coverage increased (+0.04%) to 78.565% when pulling 5d58f38 on garlick:msg_handlers into 819d8c5 on flux-framework:master.

Return array of message handlers from broker_add_services() rather than setting the result in the broker context directly, to make the function's purpose more evident from its prototype. Move handlers out of ctx to a local variable in main().

garlick · 2017-11-13T15:48:27Z

I went ahead and squashed that last change (where handlers is declared).

coveralls · 2017-11-13T16:12:07Z

Coverage increased (+0.06%) to 78.586% when pulling 43fa0c8 on garlick:msg_handlers into 819d8c5 on flux-framework:master.

garlick · 2017-11-13T17:28:03Z

Got a buildbot failure after that rebase: a recurrence of #1103 proxy event distribution (probably a racy test) seems to be the cause. There are some false positives reported by cppcheck there too, but they were there in the last build that succeeded so I guess they do not affect overall pass/fail of the build?

I would rerun the build but not sure how to do that yet...

garlick requested review from morrone and chu11 November 7, 2017 23:30

chu11 reviewed Nov 8, 2017

View reviewed changes

garlick force-pushed the msg_handlers branch from ec12939 to 51dc19f Compare November 8, 2017 16:50

garlick mentioned this pull request Nov 8, 2017

[minor cleanup]: Use different variable for flux_msg_handler_t pointers #1219

Closed

garlick force-pushed the msg_handlers branch from 51dc19f to d0ea037 Compare November 8, 2017 20:55

garlick force-pushed the msg_handlers branch from d0ea037 to 73a65bd Compare November 9, 2017 19:20

garlick mentioned this pull request Nov 10, 2017

[cleanup] use a better variable name for flux_msg_handler_t internally #1283

Merged

garlick force-pushed the msg_handlers branch from 73a65bd to 4497b47 Compare November 10, 2017 02:13

morrone suggested changes Nov 10, 2017

View reviewed changes

garlick force-pushed the msg_handlers branch from 4497b47 to ec30bff Compare November 10, 2017 23:06

garlick mentioned this pull request Nov 11, 2017

track bulk message handler registration API change flux-framework/flux-sched#278

Merged

morrone suggested changes Nov 11, 2017

View reviewed changes

garlick added 3 commits November 11, 2017 14:53

doc/flux_msg_handler_addvec(3): topic_glob is const

a3bfc4a

Problem: man page has out of date definition of struct flux_msg_handler_spec. Add "const" to topic_glob definition.

broker: unregister message handlers on exit

09c86ab

Problem: broker exits without freeing message handlers associated with internal services. Add broker_remove_service() to do this, and call it from the broker exit path.

garlick added 3 commits November 11, 2017 14:53

libjsc: drop confusing addvec parameter

26a7ba7

Problem: flux_msg_handler_addvec() is called with the 'arg' parameter set to (void *)ctx, but ctx is not yet initialized and is not used in callbacks. Set the 'arg' parameter to NULL to improve clarity.

garlick force-pushed the msg_handlers branch 2 times, most recently from 83fb802 to 08cd966 Compare November 11, 2017 14:56

garlick mentioned this pull request Nov 11, 2017

doc: add message security policy functions to man page #1285

Closed

garlick force-pushed the msg_handlers branch from 5d58f38 to 43fa0c8 Compare November 13, 2017 15:47

morrone approved these changes Nov 13, 2017

View reviewed changes

morrone merged commit 3c3890c into flux-framework:master Nov 13, 2017

garlick deleted the msg_handlers branch November 15, 2017 00:13

grondo mentioned this pull request May 10, 2018

0.9.0 Release #1479

Closed

libflux: clean up bulk message handler registration functions #1277

libflux: clean up bulk message handler registration functions #1277

Conversation

garlick commented Nov 7, 2017

coveralls commented Nov 8, 2017

codecov-io commented Nov 8, 2017 • edited Loading

Codecov Report

chu11 left a comment

Choose a reason for hiding this comment

garlick commented Nov 8, 2017

coveralls commented Nov 8, 2017

grondo commented Nov 8, 2017

garlick commented Nov 8, 2017

coveralls commented Nov 8, 2017

coveralls commented Nov 9, 2017

coveralls commented Nov 10, 2017

morrone left a 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

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

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

Choose a reason for hiding this comment

garlick commented Nov 10, 2017

morrone commented Nov 10, 2017

garlick commented Nov 10, 2017

coveralls commented Nov 10, 2017

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

garlick commented Nov 11, 2017

coveralls commented Nov 11, 2017

morrone commented Nov 12, 2017

coveralls commented Nov 12, 2017

garlick commented Nov 13, 2017

coveralls commented Nov 13, 2017

garlick commented Nov 13, 2017

codecov-io commented Nov 8, 2017 •

edited

Loading