Re: [PATCH 2/3] ADSL: Carrier Handling and PPP support

[David Woodhouse <dwmw2 infradead org>]

On Tue, 2011-05-17 at 21:03 +0300, Pantelis Koukousoulas wrote:
> +	nm_utils_complete_generic (connection,
> +	                           NM_SETTING_ADSL_SETTING_NAME,
> +	                           existing_connections,
> +	                           _("ADSL connection %d"),
> +	                           NULL,
> +	                           FALSE); /* No IPv6 yet by default */

Hm, new code in the 21st century without IPv6 support? :(

I would need that fixed before I could test this.

> +		vpi = nm_setting_adsl_get_vpi (adsl_pppoa);
> +		vci = nm_setting_adsl_get_vci (adsl_pppoa);
> +		encapsulation = nm_setting_adsl_get_encapsulation (adsl_pppoa);
> +		vpivci = g_strdup_printf("%s.%s", vpi, vci);

You want to specify device number there, not just assume there's only
one. There are dual-port PCI ADSL cards that work quite nicely in
Linux...

> +
> +		nm_cmd_line_add_string (cmd, "plugin");
> +		nm_cmd_line_add_string (cmd, "pppoatm.so");

Hm, are you also supporting PPPoE(oA)? There are some providers which
require it. Essentially the *only* thing you do on the ATM link itself
is run br2684ctl to create a 'virtual' Ethernet device. And then you do
"normal" PPPoE on that device.

> +		nm_cmd_line_add_string (cmd, vpivci);
> +
> +		if (!strcmp (encapsulation, "llc"))
> +			nm_cmd_line_add_string (cmd, "llc-encaps");
> +
> +		nm_cmd_line_add_string (cmd, "noipdefault");

Why's that unconditional? Do we not have the option to set static IP
addresses on a PPP connection? It's useful in some cases.

> +	dbus_g_error_domain_register (NM_SETTING_SERIAL_ERROR, NULL, NM_TYPE_SETTING_ADSL_ERROR);

s/SERIAL/ADSL/ ?

-- 
dwmw2