This change improves the saving of the VPN provider values to disk immediately after the value has been changed on a property. To avoid unnecessary writes to disk the changes are not written when a property change, or clearing a property, does not introduce a change to its value. Thus, PropertyChanged signal is emitted only when the property value has been changed. With any other than UserRoutes property simple value comparison is done. If the UserRoutes are changed these are read from D-Bus message into sorted list that is compared against the sorted list of saved UserRoutes. Having the lists sorted will increase performance when comparing them, especially if the lists are long. For this a compare_route() is implemented to support sorting the rules by 1) IP protocol type, 2) network addresses, 3) netmask addresses and 4) gateway addresses. The set_string() is amended to return -EALREADY when there is no change in the property value. In case the property value is changed 0 is returned. Handling of the return value is added to set_property() and clear_property() functions registered to VPN provider D-Bus interface. In both of these writing the changes to disk depends on the success of the change. Both, set_property() and clear_property() functions now return also PermissionDenied as error if immutable property is attempted to be changed. This change has the effect of not reseting errors and not setting the VPN provider to idle if it is in error state when a single property is attempted to saved without change, or an immutable value is attempted to be cleared. In order to have the state changed and errors cleared, a property value has to be changed. --- vpn/vpn-provider.c | 168 +++++++++++++++++++++++++++++++++++++++------ 1 file changed, 147 insertions(+), 21 deletions(-) diff --git a/vpn/vpn-provider.c b/vpn/vpn-provider.c index 6afcea07..bf102536 100644 --- a/vpn/vpn-provider.c +++ b/vpn/vpn-provider.c @@ -95,6 +95,7 @@ struct vpn_provider { static void append_properties(DBusMessageIter *iter, struct vpn_provider *provider); +static int vpn_provider_save(struct vpn_provider *provider); static void free_route(gpointer data) { @@ -194,6 +195,39 @@ static int provider_routes_changed(struct vpn_provider *provider) return 0; } +/* + * Sort vpn_route struct based on (similarly to the route key in hash table): + * 1) IP protocol number + * 2) Network addresses + * 3) Netmask addresses + * 4) Gateway addresses + */ +static gint compare_route(gconstpointer a, gconstpointer b) +{ + const struct vpn_route *route_a = a; + const struct vpn_route *route_b = b; + int difference; + + /* If IP families differ, prefer IPv6 over IPv4 */ + if (route_a->family != route_b->family) { + if (route_a->family < route_b->family) + return -1; + + if (route_a->family > route_b->family) + return 1; + } + + /* If networks differ, return */ + if ((difference = g_strcmp0(route_a->network, route_b->network))) + return difference; + + /* If netmasks differ, return. */ + if ((difference = g_strcmp0(route_a->netmask, route_b->netmask))) + return difference; + + return g_strcmp0(route_a->gateway, route_b->gateway); +} + static GSList *read_route_dict(GSList *routes, DBusMessageIter *dicts) { DBusMessageIter dict, value, entry; @@ -278,7 +312,7 @@ static GSList *read_route_dict(GSList *routes, DBusMessageIter *dicts) route->netmask = g_strdup(netmask); route->gateway = g_strdup(gateway); - routes = g_slist_prepend(routes, route); + routes = g_slist_insert_sorted(routes, route, compare_route); return routes; } @@ -408,6 +442,36 @@ static DBusMessage *get_properties(DBusConnection *conn, return reply; } +/* True when lists are equal, false otherwise */ +static bool compare_network_lists(GSList *a, GSList *b) +{ + struct vpn_route *route_a, *route_b; + GSList *iter_a, *iter_b; + + if (!a && !b) + return true; + + /* + * If either of lists is NULL or the lists are of different size, the + * lists are not equal. + */ + if ((!a || !b) || (g_slist_length(a) != g_slist_length(b))) + return false; + + /* Routes are in sorted list so items can be compared in order. */ + for (iter_a = a, iter_b = b; iter_a && iter_b; + iter_a = iter_a->next, iter_b = iter_b->next) { + + route_a = iter_a->data; + route_b = iter_b->data; + + if (compare_route(route_a, route_b)) + return false; + } + + return true; +} + static DBusMessage *set_property(DBusConnection *conn, DBusMessage *msg, void *data) { @@ -415,6 +479,7 @@ static DBusMessage *set_property(DBusConnection *conn, DBusMessage *msg, DBusMessageIter iter, value; const char *name; int type; + int err = 0; DBG("conn %p", conn); @@ -444,15 +509,19 @@ static DBusMessage *set_property(DBusConnection *conn, DBusMessage *msg, return __connman_error_invalid_arguments(msg); networks = get_user_networks(&value); - if (networks) { - del_routes(provider); - provider->user_networks = networks; - set_user_networks(provider, provider->user_networks); + if (!networks) + return __connman_error_invalid_arguments(msg); - if (!handle_routes) - send_routes(provider, provider->user_routes, - "UserRoutes"); - } + if (compare_network_lists(provider->user_networks, networks)) + return g_dbus_create_reply(msg, DBUS_TYPE_INVALID); + + del_routes(provider); + provider->user_networks = networks; + set_user_networks(provider, provider->user_networks); + + if (!handle_routes) + send_routes(provider, provider->user_routes, + "UserRoutes"); } else { const char *str; @@ -460,7 +529,20 @@ static DBusMessage *set_property(DBusConnection *conn, DBusMessage *msg, return __connman_error_invalid_arguments(msg); dbus_message_iter_get_basic(&value, &str); - vpn_provider_set_string(provider, name, str); + + err = vpn_provider_set_string(provider, name, str); + } + + switch (err) { + case 0: + vpn_provider_save(provider); + break; + case -EALREADY: + break; + case -EINVAL: + return __connman_error_invalid_property(msg); + default: + return __connman_error_failed(msg, -err); } return g_dbus_create_reply(msg, DBUS_TYPE_INVALID); @@ -471,6 +553,8 @@ static DBusMessage *clear_property(DBusConnection *conn, DBusMessage *msg, { struct vpn_provider *provider = data; const char *name; + bool change = false; + int err; DBG("conn %p", conn); @@ -481,16 +565,38 @@ static DBusMessage *clear_property(DBusConnection *conn, DBusMessage *msg, DBUS_TYPE_INVALID); if (g_str_equal(name, "UserRoutes")) { + /* + * If either user_routes or user_networks has any entries + * there is a change that is to be written to settings file. + */ + if (g_hash_table_size(provider->user_routes) || + provider->user_networks) + change = true; + del_routes(provider); if (!handle_routes) send_routes(provider, provider->user_routes, name); } else if (vpn_provider_get_string(provider, name)) { - vpn_provider_set_string(provider, name, NULL); + err = vpn_provider_set_string(provider, name, NULL); + switch (err) { + case 0: + change = true; + /* fall through */ + case -EALREADY: + break; + case -EINVAL: + return __connman_error_invalid_property(msg); + default: + return __connman_error_failed(msg, -err); + } } else { return __connman_error_invalid_property(msg); } + if (change) + vpn_provider_save(provider); + return g_dbus_create_reply(msg, DBUS_TYPE_INVALID); } @@ -2302,35 +2408,54 @@ static int set_string(struct vpn_provider *provider, hide_value ? "<not printed>" : value); if (g_str_equal(key, "Type")) { + if (!g_strcmp0(provider->type, value)) + return -EALREADY; + g_free(provider->type); provider->type = g_ascii_strdown(value, -1); send_value(provider->path, "Type", provider->type); } else if (g_str_equal(key, "Name")) { + if (!g_strcmp0(provider->name, value)) + return -EALREADY; + g_free(provider->name); provider->name = g_strdup(value); send_value(provider->path, "Name", provider->name); } else if (g_str_equal(key, "Host")) { + if (!g_strcmp0(provider->host, value)) + return -EALREADY; + g_free(provider->host); provider->host = g_strdup(value); send_value(provider->path, "Host", provider->host); } else if (g_str_equal(key, "VPN.Domain") || g_str_equal(key, "Domain")) { + if (!g_strcmp0(provider->domain, value)) + return -EALREADY; + g_free(provider->domain); provider->domain = g_strdup(value); send_value(provider->path, "Domain", provider->domain); } else { struct vpn_setting *setting; + bool replace = true; setting = g_hash_table_lookup(provider->setting_strings, key); - if (setting && !immutable && - setting->immutable) { - DBG("Trying to set immutable variable %s", key); - return -EPERM; - } + if (setting) { + if (!immutable && setting->immutable) { + DBG("Trying to set immutable variable %s", key); + return -EPERM; + } else if (!g_strcmp0(setting->value, value)) { + return -EALREADY; + } - setting = g_try_new0(struct vpn_setting, 1); - if (!setting) - return -ENOMEM; + g_free(setting->value); + replace = false; + } else { + setting = g_try_new0(struct vpn_setting, 1); + if (!setting) + return -ENOMEM; + } setting->value = g_strdup(value); setting->hide_value = hide_value; @@ -2341,8 +2466,9 @@ static int set_string(struct vpn_provider *provider, if (!hide_value) send_value(provider->path, key, setting->value); - g_hash_table_replace(provider->setting_strings, - g_strdup(key), setting); + if (replace) + g_hash_table_replace(provider->setting_strings, + g_strdup(key), setting); } return 0; -- 2.20.1

Add SetProperties D-Bus method (set_properties()) to allow setting all VPN properties with one D-Bus call. This improves the performance when multiple (or all) properties are to be changed but reduces the granularity of errors reported back to the caller. Properties are accepted in the same format as with GetProperties D-Bus method sends them, a{sv}. Empty strings as property values, or empty array for UserRoutes will clear the property value. If there is at least one invalid property, then InvalidProperty D-Bus error is sent as reply. Invalid properties > immutable properties in case of error, there can be permission errors caused by immutable properties when InvalidProperty error is sent. Only when there are only permission errors then PermissionDenied D-Bus error is sent. In both cases the D-Bus error message is amended with the property names that caused the errors. Move setting of a single property into its own function, usable by both set_property() and set_properties(). The set_vpn_property() retains the functionality by changing either the routes or a single string value, returns 0 when success, -EALREADY when there is no change on property value and appropriate error (-EINVAL/-EPERM/-ENOMEM) otherwise. --- vpn/vpn-provider.c | 208 ++++++++++++++++++++++++++++++++++++++------- 1 file changed, 176 insertions(+), 32 deletions(-) diff --git a/vpn/vpn-provider.c b/vpn/vpn-provider.c index bf102536..b01da31d 100644 --- a/vpn/vpn-provider.c +++ b/vpn/vpn-provider.c @@ -472,6 +472,177 @@ static bool compare_network_lists(GSList *a, GSList *b) return true; } +static int set_provider_property(struct vpn_provider *provider, + const char *name, DBusMessageIter *value, int type, + bool clear_if_empty) +{ + int err = 0; + + DBG("provider %p", provider); + + if (!provider || !name || !value) + return -EINVAL; + + if (g_str_equal(name, "UserRoutes")) { + GSList *networks; + + if (type != DBUS_TYPE_ARRAY) + return -EINVAL; + + networks = get_user_networks(value); + if (!networks && !clear_if_empty) + return -EINVAL; + + if (compare_network_lists(provider->user_networks, networks)) { + g_slist_free_full(networks, free_route); + return -EALREADY; + } + + del_routes(provider); + provider->user_networks = networks; + set_user_networks(provider, provider->user_networks); + + if (!handle_routes) + send_routes(provider, provider->user_routes, + "UserRoutes"); + } else { + const char *str; + + if (type != DBUS_TYPE_STRING) + return -EINVAL; + + dbus_message_iter_get_basic(value, &str); + + DBG("property %s value %s", name, str); + + /* + * Empty strings must not be allowed unless explicitely set to + * be cleared if the string is empty. If the value is to be + * cleared ClearProperty D-Bus method must be used. + */ + if (!*str && !clear_if_empty) + return -EINVAL; + + err = vpn_provider_set_string(provider, name, + *str ? str : NULL); + } + + return err; +} + +static GString *append_to_gstring(GString *str, const char *value) +{ + if (!str) + return g_string_new(value); + + g_string_append_printf(str, ",%s", value); + + return str; +} + +static DBusMessage *set_properties(DBusConnection *conn, DBusMessage *msg, + void *data) +{ + struct vpn_provider *provider = data; + DBusMessageIter iter, dict; + const char *key; + bool change = false; + GString *invalid = NULL; + GString *denied = NULL; + int type; + int err; + + DBG("conn %p", conn); + + if (provider->immutable) + return __connman_error_not_supported(msg); + + if (!dbus_message_iter_init(msg, &iter)) + return __connman_error_invalid_arguments(msg); + + for (dbus_message_iter_recurse(&iter, &dict); + dbus_message_iter_get_arg_type(&dict) == + DBUS_TYPE_DICT_ENTRY; + dbus_message_iter_next(&dict)) { + DBusMessageIter entry, value; + + dbus_message_iter_recurse(&dict, &entry); + /* + * Ignore invalid types in order to process all values in the + * dict. If there is an invalid type in between the dict there + * may already be changes on some values and breaking out here + * would have the provider in an inconsistent state, leaving + * the rest, potentially correct property values untouched. + */ + if (dbus_message_iter_get_arg_type(&entry) != DBUS_TYPE_STRING) + continue; + + dbus_message_iter_get_basic(&entry, &key); + + DBG("key %s", key); + + dbus_message_iter_next(&entry); + /* Ignore and report back all non variant types. */ + if (dbus_message_iter_get_arg_type(&entry) + != DBUS_TYPE_VARIANT) { + invalid = append_to_gstring(invalid, key); + continue; + } + + dbus_message_iter_recurse(&entry, &value); + + type = dbus_message_iter_get_arg_type(&value); + /* Ignore and report back all invalid property types */ + if (type != DBUS_TYPE_STRING && type != DBUS_TYPE_ARRAY) { + invalid = append_to_gstring(invalid, key); + continue; + } + + err = set_provider_property(provider, key, &value, type, true); + switch (err) { + case 0: + change = true; + break; + case -EINVAL: + invalid = append_to_gstring(invalid, key); + break; + case -EPERM: + denied = append_to_gstring(denied, key); + break; + } + } + + if (change) + vpn_provider_save(provider); + + if (invalid || denied) { + DBusMessage *error; + char *invalid_str = g_string_free(invalid, FALSE); + char *denied_str = g_string_free(denied, FALSE); + + /* + * If there are both invalid and denied properties report + * back invalid arguments. Add also the failed properties to + * the error message. + */ + error = g_dbus_create_error(msg, (invalid ? + CONNMAN_ERROR_INTERFACE ".InvalidProperty" : + CONNMAN_ERROR_INTERFACE ".PermissionDenied"), + "%s %s%s%s", (invalid ? "Invalid properties" : + "Permission denied"), + (invalid ? invalid_str : ""), + (invalid && denied ? "," : ""), + (denied ? denied_str : "")); + + g_free(invalid_str); + g_free(denied_str); + + return error; + } + + return g_dbus_create_reply(msg, DBUS_TYPE_INVALID); +} + static DBusMessage *set_property(DBusConnection *conn, DBusMessage *msg, void *data) { @@ -479,7 +650,7 @@ static DBusMessage *set_property(DBusConnection *conn, DBusMessage *msg, DBusMessageIter iter, value; const char *name; int type; - int err = 0; + int err; DBG("conn %p", conn); @@ -502,37 +673,7 @@ static DBusMessage *set_property(DBusConnection *conn, DBusMessage *msg, type = dbus_message_iter_get_arg_type(&value); - if (g_str_equal(name, "UserRoutes")) { - GSList *networks; - - if (type != DBUS_TYPE_ARRAY) - return __connman_error_invalid_arguments(msg); - - networks = get_user_networks(&value); - if (!networks) - return __connman_error_invalid_arguments(msg); - - if (compare_network_lists(provider->user_networks, networks)) - return g_dbus_create_reply(msg, DBUS_TYPE_INVALID); - - del_routes(provider); - provider->user_networks = networks; - set_user_networks(provider, provider->user_networks); - - if (!handle_routes) - send_routes(provider, provider->user_routes, - "UserRoutes"); - } else { - const char *str; - - if (type != DBUS_TYPE_STRING) - return __connman_error_invalid_arguments(msg); - - dbus_message_iter_get_basic(&value, &str); - - err = vpn_provider_set_string(provider, name, str); - } - + err = set_provider_property(provider, name, &value, type, false); switch (err) { case 0: vpn_provider_save(provider); @@ -640,6 +781,9 @@ static const GDBusMethodTable connection_methods[] = { { GDBUS_METHOD("GetProperties", NULL, GDBUS_ARGS({ "properties", "a{sv}" }), get_properties) }, + { GDBUS_METHOD("SetProperties", + GDBUS_ARGS({ "properties", "a{sv}" }), + NULL, set_properties) }, { GDBUS_METHOD("SetProperty", GDBUS_ARGS({ "name", "s" }, { "value", "v" }), NULL, set_property) }, -- 2.20.1

Include documentation for the use of dict array with the SetProperty D-Bus method in VPN connection API. Add error PermissionDenied for SetProperty and ClearProperty into VPN connection API. Add missing NotSupported error for SetProperty into VPN connection API. --- Changes since v2: * Removed SetProperties D-Bus method description from API. * Reformatted the SetProperties description under SetProperty description in the API. * Changed the commit title to be more appropriate. doc/vpn-connection-api.txt | 43 ++++++++++++++++++++++++++++++++++---- 1 file changed, 39 insertions(+), 4 deletions(-) diff --git a/doc/vpn-connection-api.txt b/doc/vpn-connection-api.txt index 1fd3be26..80080ebe 100644 --- a/doc/vpn-connection-api.txt +++ b/doc/vpn-connection-api.txt @@ -14,13 +14,47 @@ Methods dict GetProperties() [experimental] void SetProperty(string name, variant value) [experimental] - Changes the value of the specified property. Only - properties that are listed as read-write are - changeable. On success a PropertyChanged signal - will be emitted. + Changes the value of the specified property or the + properties defined as a dict passed as variant, where + the format is equal to the dict returned by + GetProperties(). Only properties that are listed as + read-write are changeable. On success a + PropertyChanged signal will be emitted for the + specified property or for all changed properties + individually. If there is no change in property value + no PropertyChanged signal is sent. Configuration is + written to disk when one or more values are changed. + In case a dict of properties are given, configuration + write is done after all properties are processed. + Specifics in dict use in contrast to setting a single + property: + - Dict can contain values set as empty strings + or arrays. This causes the values to be + cleared as if using ClearProperty(). + - If there are errors with the properties, + InvalidProperty or PermissionDenied error is + returned. InvalidProperty is sent when there + is at least one invalid property, in this + case there can be also properties that + cannot be changed (immutable properties). + If there are only immutable properties + PermissionDenied error is returned. + - The properties that are invalid or immutable + are reported back at the end of the error + message as a comma separated property name + list. + - One invalid/immutable property does not + cause the rest of the properties to be + ignored. If there are valid and invalid + properties, the valid properties emit + PropertyChanged signal and invalid are + reported back with an InvalidProperty + message. Possible Errors: [connection].Error.InvalidArguments [connection].Error.InvalidProperty + [connection].Error.PermissionDenied + [connection].Error.NotSupported void ClearProperty(string name) [experimental] @@ -28,6 +62,7 @@ Methods dict GetProperties() [experimental] Possible Errors: [connection].Error.InvalidArguments [connection].Error.InvalidProperty + [connection].Error.PermissionDenied void Connect() [experimental] -- 2.20.1