[systemd-devel] [PATCH] core: support Distribute=n to distribute to n workers

[Lennart Poettering]

On Tue, 10.12.13 18:53, Shawn Landden (shawn at churchofgit.com) wrote:

> @@ -116,6 +115,7 @@ const sd_bus_vtable bus_socket_vtable[] = {
>          SD_BUS_PROPERTY("MessageQueueMessageSize", "x", bus_property_get_long, offsetof(Socket, mq_msgsize), 0),
>          SD_BUS_PROPERTY("Result", "s", property_get_result, offsetof(Socket, result), SD_BUS_VTABLE_PROPERTY_EMITS_CHANGE),
>          SD_BUS_PROPERTY("ReusePort", "b",  bus_property_get_bool,
>          offsetof(Socket, reuse_port), 0),

Did you turn reuseport into a tristate as suggested? If so, then
bus_property_get_bool needs to become bus_property_get_tristate here so
that the right type is serialized to dbus.

> +Socket.ReusePort,                config_parse_bool,                 -1,                             offsetof(Socket, reuse_port)

Similar here... (and what's the -1?)

As it turns out I recently removed config_parse_tristate() as part of
9588bc32096fc8342bfd8b989689717186d7d86e. You should probably restore
that function from that commit to make use of it here...


> +                Socket *socket = SOCKET(UNIT_DEREF(s->accept_socket));
>                  log_debug_unit(u->id,
>                                 "%s: got READY=1", u->id);
>  
> +                if (socket && socket->distribute > socket->n_connections)
> +                        socket_enter_listening(socket);
> +

Hmm, I would prefer if we wouldn't reach over too much into the socket
here, i.e. handle this within socket_trigger_notify(). That function is
always called when a unit that is triggered by the socket changes state.

> +        if (s->reuse_port == -1) {

I'd prefer not checking against specific values,but really just if <
0, > 0, or == 0. Checking against specific values would kinda suggest
that there are more than three values.

if (s->reuse_port < 0) ...

Also, please do this part as part of socket_load() somewhere, so that
the < 0 state is turned into > 0 or == 0 already when loading the unit.

> -        if (cfd < 0) {
> +        if (cfd < 0 && !(s->distribute)) {

The extra ()... Also, please use == 0 here since it is a number...

(Same fix at a few other places...)

> +                if (s->distribute > 0 && (s->n_connections >= s->distribute || SERVICE(UNIT_DEREF(s->service))->type == SERVICE_NOTIFY))
> +                        socket_set_state(s, SOCKET_RUNNING);
> +

Why do you check for SERVICE_NOTIFY here?

Also, maybe I missed something, but if SO_REUSEPORT is used you need to
create a new socket for each connection and bind it to the same
address. You have to duplicate the existing socket, but not with dup(),
but instead by creating a new one with the same parameters and with
SO_REUSEPORT... The first instance this spawns should get the original
socket though, but all others should get one of these copies...

Lennart

-- 
Lennart Poettering, Red Hat