On Tue, Aug 02, 2016 at 02:01:05AM +0530, Atul Anand wrote: > libnm-core has been expanded to include proxy settings which clients > like nmcli, nm-connection-editor use to configure proxy in PacRunner. It > offers three modes i.e 'auto', 'manual'and 'none' and accordingly take > data to configure PacRunner. The modes matches on the PacRunner side too.
Hi,
the series looks good in general. Some comments below.
> --- a/libnm-core/nm-connection.c
> +++ b/libnm-core/nm-connection.c
> @@ -2137,6 +2137,22 @@ nm_connection_get_setting_pppoe (NMConnection
> *connection)
> }
>
> /**
> + * nm_connection_get_setting_proxy:
> + * @connection: the #NMConnection
> + *
> + * A shortcut to return any #NMSettingProxy the connection might contain.
> + *
> + * Returns:an #NMSettingProxy if the connection contains one, otherwise %NULL
Nitpick: this is public API and needs the "Since" tag...
> --- a/libnm-core/nm-connection.h
> +++ b/libnm-core/nm-connection.h
> @@ -211,6 +211,7 @@ NMSettingMacvlan *
> nm_connection_get_setting_macvlan (NMConnec
> NMSettingOlpcMesh * nm_connection_get_setting_olpc_mesh
> (NMConnection *connection);
> NMSettingPpp * nm_connection_get_setting_ppp
> (NMConnection *connection);
> NMSettingPppoe * nm_connection_get_setting_pppoe
> (NMConnection *connection);
> +NMSettingProxy * nm_connection_get_setting_proxy
> (NMConnection *connection);
... and NM_AVAILABLE_IN_1_2 here.
> + if (method == NM_SETTING_PROXY_METHOD_NONE) {
> + if (priv->pac_url) {
> + g_set_error (error,
> + NM_CONNECTION_ERROR,
> +
> NM_CONNECTION_ERROR_INVALID_PROPERTY,
> + _("this property is not allowed
> for method=manual/none"));
"for method none"
> +
> + if (priv->pac_script) {
> + g_set_error (error,
> + NM_CONNECTION_ERROR,
> +
> NM_CONNECTION_ERROR_INVALID_PROPERTY,
> + _("this property is not allowed
> for method=manual/none"));
Ditto.
> + } else if (method == NM_SETTING_PROXY_METHOD_MANUAL) {
> + if (priv->pac_url) {
> + g_set_error (error,
> + NM_CONNECTION_ERROR,
> + NM_CONNECTION_ERROR_INVALID_PROPERTY,
> + _("this property is not allowed for
> method=manual/none"));
"for method manual".
> + if (priv->pac_script) {
> + g_set_error (error,
> + NM_CONNECTION_ERROR,
> + NM_CONNECTION_ERROR_INVALID_PROPERTY,
> + _("this property is not allowed for
> method=manual/none"));
Ditto
> + g_prefix_error (error, "%s.%s: ",
> NM_SETTING_PROXY_SETTING_NAME, NM_SETTING_PROXY_PAC_SCRIPT);
> + return FALSE;
> + }
> + } else
> + return FALSE;
@error must be always set when returning FALSE.
> +static void
> +set_property (GObject *object, guint prop_id,
> + const GValue *value, GParamSpec *pspec)
> + [...]
> + case PROP_SSL_PROXY:
> + g_free (priv->ssl_proxy);
> + /* Check if HTTP Proxy has been set for all Protocols */
> + priv->ssl_proxy = priv->http_default == TRUE ? g_strdup
> (priv->http_proxy) : g_value_dup_string (value);
In my opinion here we should simply assign @value to priv->ssl_proxy;
the fallback to default should be something done by the caller. But
if we really must do it here, @http_default is already a gboolean and
so the "== TRUE" is not needed.
Beniamino
signature.asc
Description: PGP signature
_______________________________________________ networkmanager-list mailing list networkmanager-list@gnome.org https://mail.gnome.org/mailman/listinfo/networkmanager-list
