Hi Tomasz,
> Here is a patch for pacrunner plugin. I tested it (against patched pacrunner
> which handles manual config - the previous patch I sent for pacrunner daemon)
> and it works fine.
> So now it handles direct/manual/auto as specified.
>
> However, pacrunner advertises proxy configuration only when service change
> occurs, would be better if it also does this when proxy has changed on this
> same service. Have to investigate for this.
Yes, I saw that being a problem actually. We need to have a new notifier
that does service_ready or service_changed.
The default_changed notifier is not really working out for us. And we
wanna load proxy configuration into PACrunner for all systems when the
service reached READY state.
That said, we need to delay the READY state until we finished WPAD
resolving. And in case DHCP does not give us the PAC file.
> include/service.h | 4 ++
> plugins/pacrunner.c | 77 ++++++++++++++++++++++++++++++++++++++++++--------
> src/service.c | 30 +++++++++++++++++++-
> 3 files changed, 97 insertions(+), 14 deletions(-)
I only have a few nitpicks.
First split this into two patches. One that adds the extra service
functions, and the second that changes pacrunner plugin.
> diff --git a/include/service.h b/include/service.h
> index 8c0e892..16d10f2 100644
> --- a/include/service.h
> +++ b/include/service.h
> @@ -102,6 +102,10 @@ char *connman_service_get_interface(struct
> connman_service *service);
>
> const char *connman_service_get_domainname(struct connman_service *service);
> const char *connman_service_get_nameserver(struct connman_service *service);
> +enum connman_service_proxy_method connman_service_get_proxy_method(struct
> connman_service *service);
> +char **connman_service_get_proxy_servers(struct connman_service *service);
> +char **connman_service_get_proxy_excludes(struct connman_service *service);
> +const char *connman_service_get_proxy_url(struct connman_service *service);
> const char *connman_service_get_proxy_autoconfig(struct connman_service
> *service);
>
> #ifdef __cplusplus
> diff --git a/plugins/pacrunner.c b/plugins/pacrunner.c
> index 3a21011..c9bb119 100644
> --- a/plugins/pacrunner.c
> +++ b/plugins/pacrunner.c
> @@ -73,6 +73,16 @@ static void append_string(DBusMessageIter *iter, void
> *user_data)
> dbus_message_iter_append_basic(iter, DBUS_TYPE_STRING, user_data);
> }
>
> +static void append_string_list(DBusMessageIter *iter, void *user_data)
> +{
> + int i;
> + char **list = user_data;
General rule for coding style here is that user_data assignment is the
first variable in the list.
And second is that int i should be always the last one.
We might not be 100% consistent in the code, but I was pretty strict
with I would assume 98% of the code to enforce this.
> +
> + for (i = 0; list[i] != NULL; i++)
> + dbus_message_iter_append_basic(iter,
> + DBUS_TYPE_STRING, &list[i]);
> +}
> +
> static void create_proxy_configuration(void)
> {
> DBusMessage *msg;
> @@ -80,7 +90,9 @@ static void create_proxy_configuration(void)
> DBusPendingCall *call;
> dbus_bool_t result;
> char *interface;
> + const char *method;
> const char *str;
> + char **str_list;
>
> if (default_service == NULL)
> return;
> @@ -97,6 +109,58 @@ static void create_proxy_configuration(void)
> dbus_message_iter_init_append(msg, &iter);
> connman_dbus_dict_open(&iter, &dict);
>
> + switch(connman_service_get_proxy_method(default_service)) {
> + case CONNMAN_SERVICE_PROXY_METHOD_UNKNOWN:
> + /* fall through */
> + case CONNMAN_SERVICE_PROXY_METHOD_DIRECT:
> + method= "direct";
> + break;
I don't like the fall through to direct here. Just abort and not load
anything into pacrunner. It will fallback to direct, but in that case it
is pacrunners choice/policy. ConnMan should not decide here. The reason
for having METHOD_UNKNOWN to handle it differently. Otherwise it would
be rather pointless to have it in the first place.
> + case CONNMAN_SERVICE_PROXY_METHOD_MANUAL:
> + method = "manual";
> +
> + str_list = connman_service_get_proxy_servers(default_service);
> + if (str_list != NULL) {
> + connman_dbus_dict_append_array(&dict, "Servers",
> + DBUS_TYPE_STRING, append_string_list,
> + str_list);
> +
> + g_strfreev(str_list);
> + }
> + else {
It is } else {
> + method = "direct";
> + break;
> + }
And to be honest. Just doing
if (str_list == NULL) {
method = "direct"
break;
}
connman_dbus_....
g_strfreev(...
is a lot simpler to read.
> +
> + str_list = connman_service_get_proxy_excludes(default_service);
> + if (str_list != NULL) {
> + connman_dbus_dict_append_array(&dict, "Excludes",
> + DBUS_TYPE_STRING, append_string_list,
> + str_list);
> +
> + g_strfreev(str_list);
> + }
This case is not that critical, but again, I would prefer
if (str_list == NULL)
breakl
connman_dbus....
to make it consistent with the one above.
> +
> + break;
> + case CONNMAN_SERVICE_PROXY_METHOD_AUTO:
> + method = "auto";
> +
> + str = connman_service_get_proxy_url(default_service);
> + if (str == NULL)
> + str = connman_service_get_proxy_autoconfig(
> + default_service);
> +
> + if (str != NULL)
> + connman_dbus_dict_append_basic(&dict, "URL",
> + DBUS_TYPE_STRING, &str);
> + else
> + method = "direct";
See comments above.
Also right now I am thinking if we have a misconfiguration, then we
should just not load any configuration into pacrunner. Lets just abort
and leave pacrunner blank. As explained above, then we can make proper
decisions in pacrunner.
> +
> + break;
> + }
> +
> + connman_dbus_dict_append_basic(&dict, "Method",
> + DBUS_TYPE_STRING, &method);
> +
> interface = connman_service_get_interface(default_service);
> if (interface != NULL) {
> connman_dbus_dict_append_basic(&dict, "Interface",
> @@ -104,19 +168,6 @@ static void create_proxy_configuration(void)
> g_free(interface);
> }
>
> - str = connman_service_get_proxy_autoconfig(default_service);
> - if (str != NULL) {
> - const char *method = "auto";
> - connman_dbus_dict_append_basic(&dict, "Method",
> - DBUS_TYPE_STRING, &method);
> - connman_dbus_dict_append_basic(&dict, "URL",
> - DBUS_TYPE_STRING, &str);
> - } else {
> - const char *method = "direct";
> - connman_dbus_dict_append_basic(&dict, "Method",
> - DBUS_TYPE_STRING, &method);
> - }
> -
> str = connman_service_get_domainname(default_service);
> if (str != NULL)
> connman_dbus_dict_append_array(&dict, "Domains",
> diff --git a/src/service.c b/src/service.c
> index 1cb292f..65efc34 100644
> --- a/src/service.c
> +++ b/src/service.c
> @@ -1527,6 +1527,33 @@ const char *connman_service_get_nameserver(struct
> connman_service *service)
> return service->nameserver;
> }
>
> +enum connman_service_proxy_method connman_service_get_proxy_method(
> + struct connman_service *service)
> +{
> + if (service == NULL)
> + return CONNMAN_SERVICE_PROXY_METHOD_UNKNOWN;
> +
> + return service->proxy;
> +}
> +
> +char **connman_service_get_proxy_servers(struct connman_service *service)
> +{
> + return g_strdupv(service->proxies);
> +}
> +
> +char **connman_service_get_proxy_excludes(struct connman_service *service)
> +{
> + return g_strdupv(service->excludes);
> +}
In theory we could just return the reference here, but I did have some
issues with const string arrays before. Maybe this is just the better
approach.
> +const char *connman_service_get_proxy_url(struct connman_service *service)
> +{
> + if (service == NULL)
> + return NULL;
> +
> + return service->pac;
> +}
> +
> void __connman_service_set_proxy_autoconfig(struct connman_service *service,
> const char *url)
> {
> @@ -1540,7 +1567,8 @@ void __connman_service_set_proxy_autoconfig(struct
> connman_service *service,
> proxy_changed(service);
> }
>
> -const char *connman_service_get_proxy_autoconfig(struct connman_service
> *service)
> +const char *connman_service_get_proxy_autoconfig(
> + struct connman_service *service)
This case is fine to violate the 78 chars per line rule. That is the one
exception ;)
> {
> if (service == NULL)
> return NULL;
Regards
Marcel
_______________________________________________
connman mailing list
[email protected]
http://lists.connman.net/listinfo/connman
