[systemd-devel] [RFC 4/6] proxy-discoveryd: Execute the PAC based proxy in a thread

[Lennart Poettering]

On Fri, 10.04.15 15:17, Tomasz Bursztyka (tomasz.bursztyka at linux.intel.com) wrote:

> +static void *run_thread(void *data) {
> +        _cleanup_free_ struct proxy_parameters *params = NULL;
> +        _cleanup_pac_free_ struct PAC *pac = NULL;
> +        _cleanup_free_ char *result = NULL;
> +        const char *url, *host;
> +
> +        params = data;
> +
> +        if (sd_bus_message_read(params->message, "ss", &url, &host) < 0)
> +                goto answer;
> +
> +        if (pac_load(params->proxy, &pac) < 0)
> +                goto answer;
> +
> +        pac_execute(pac, url, host, &result);
> +
> +answer:
> +        if (!result)
> +                result = strdup("DIRECT");
> +
> +        sd_bus_reply_method_return(params->message, "s", result);

sd-bus is not thread-safe. You cannot send messages from different
threads at the same time...

Why precisely do you need threads here? Because the PAC programs might
be slow?

WHat about unbounded loops in PAC programs? We need to protect from
that, which kinda suggests we muight want to run the PAC stuff out of
process so that we can neatly kill the program when it runs for too
long?

> +
> +        params->message = sd_bus_message_unref(params->message);
> +
> +        return NULL;
> +}
> +
> +int proxy_execute(Proxy *proxy, sd_bus_message *message) {
> +        _cleanup_free_ struct proxy_parameters *params = NULL;
> +        int r;
> +
> +        if (!proxy)
> +                return -EINVAL;
> +
> +        params = new0(struct proxy_parameters, 1);
> +        if (!params)
> +                return log_oom();

Hmm, so we generally try to write functions so that they log all
errors on their own, or none (so that the caller can log instead). In
the upper error condition you fail with -EINVAL without logging, in
the lower error condition you fail with -ENOMEM but log. Please decide
whether the function should log all errors or none, and make the
caller handle this nicely. Otherwise we might easily get into cases
where errors are logged twice or none at all.

Usually for more "library-like" calls we leave the logging to the
caller, and for "main-program-like" calls we log in the calls
themselves, but the line between these two cases are blurry...

Lennart

-- 
Lennart Poettering, Red Hat