Re: issues with pppd error handling

[Beniamino Galvani <bgalvani redhat com>]

On Mon, Nov 27, 2017 at 03:52:26PM +0100, Piotr Figiel wrote:
> Is it really needed to receive and process notifications about
> internal pppd state changes? Since pppd is/should be running in
> nopersist mode pppd should exit upon connection termination. Actually
> there were only two 'live' cases where this information is used in NM
> and both are related with disconnect and cause ppp_failed() to run as
> mentioned in my previous message.

The phase state change information is needed in several places to
determine why pppd failed. It is also needed in NMDevicePpp to start
the IP configuration when pppd enters the 'running' phase.

> I attach patch I'm using now and it works OK for pppd with
> ModemManager. I don't think it's directly upstreamable as it affects
> pppoe and bluetooth (which have ppp_failed() copy-pasted) which I
> can't test now and which would potentially be harmed by this patch but
> maybe illustrates potential solution.

> diff --git a/src/devices/wwan/nm-device-modem.c b/src/devices/wwan/nm-device-modem.c
> index 4a4d2f2..245cd58 100644
> --- a/src/devices/wwan/nm-device-modem.c
> +++ b/src/devices/wwan/nm-device-modem.c
> @@ -76,38 +76,7 @@ ppp_failed (NMModem *modem,
>       NMDeviceModem *self = NM_DEVICE_MODEM (user_data);
>       NMDeviceStateReason reason = i_reason;
>  
> -     switch (nm_device_get_state (device)) {
> -     case NM_DEVICE_STATE_PREPARE:
> -     case NM_DEVICE_STATE_CONFIG:
> -     case NM_DEVICE_STATE_NEED_AUTH:
> -             nm_device_state_changed (device, NM_DEVICE_STATE_FAILED, reason);
> -             break;
> -     case NM_DEVICE_STATE_IP_CONFIG:
> -     case NM_DEVICE_STATE_IP_CHECK:
> -     case NM_DEVICE_STATE_SECONDARIES:
> -     case NM_DEVICE_STATE_ACTIVATED:
> -             if (nm_device_activate_ip4_state_in_conf (device))
> -                     nm_device_activate_schedule_ip4_config_timeout (device);
> -             else if (nm_device_activate_ip6_state_in_conf (device))
> -                     nm_device_activate_schedule_ip6_config_timeout (device);
> -             else if (nm_device_activate_ip4_state_done (device)) {
> -                     nm_device_ip_method_failed (device,
> -                                                 AF_INET,
> -                                                 NM_DEVICE_STATE_REASON_IP_CONFIG_UNAVAILABLE);
> -             } else if (nm_device_activate_ip6_state_done (device)) {
> -                     nm_device_ip_method_failed (device,
> -                                                 AF_INET6,
> -                                                 NM_DEVICE_STATE_REASON_IP_CONFIG_UNAVAILABLE);
> -             } else {
> -                     _LOGW (LOGD_MB, "PPP failure in unexpected state %u", (guint) nm_device_get_state 
> (device));
> -                     nm_device_state_changed (device,
> -                                              NM_DEVICE_STATE_FAILED,
> -                                              NM_DEVICE_STATE_REASON_IP_CONFIG_UNAVAILABLE);
> -             }
> -             break;
> -     default:
> -             break;
> -     }
> +     nm_device_state_changed (device, NM_DEVICE_STATE_FAILED, reason);
>  }


This looks correct to me because when pppd fails the whole connection
shold fail, not just single IP methods.

Dan, what do you think?

>  static void
> diff --git a/src/ppp/nm-pppd-plugin.c b/src/ppp/nm-pppd-plugin.c
> index 9c47c33..9b22679 100644
> --- a/src/ppp/nm-pppd-plugin.c
> +++ b/src/ppp/nm-pppd-plugin.c
> @@ -408,7 +408,6 @@ plugin_init (void)
>       pap_passwd_hook = get_credentials;
>       pap_check_hook = get_pap_check;
>  
> -     add_notifier (&phasechange, nm_phasechange, NULL);
>       add_notifier (&ip_up_notifier, nm_ip_up, NULL);
>       add_notifier (&exitnotify, nm_exit_notify, proxy);
>       add_ip6_notifier ();

As explained above the notifier is needed.

Beniamino

Attachment: signature.asc
Description: PGP signature