Re: [PATCH] modem: map modemmanager errors more detailed



On Sun, 2011-11-13 at 19:06 +0100, Thomas Bechtold wrote:
> Hi,
> 
> attached is a patch to handle some ModemManager errors more detailed.
> But i still have some questions about the code and i'm not sure if this
> patch is the correct way to solve the problem that NM does not know the
> exact reason why a modem connection fails.
> Here are my questions:
> 
> - should NM have a NMDeviceStateReason for every MM error 
>   or should some MM errors be combined to one NMDeviceStateReason?

Probably not every reason, but at least the most important ones.

> - in NM/include/NetworkManager.h are some NMDeviceStateReason defined 
>   which are never used. eg NM_DEVICE_STATE_REASON_MODEM_NO_DIAL_TONE, 
>   NM_DEVICE_STATE_REASON_MODEM_NO_CARRIER, 
>   NM_DEVICE_STATE_REASON_MODEM_DIAL_FAILED, 
>   NM_DEVICE_STATE_REASON_GSM_APN_FAILED, 
>   NM_DEVICE_STATE_REASON_GSM_PIN_CHECK_FAILED
>   maybe there are some more unused NMDeviceStateReason. 
>   Why are these reason available?

Because we thought they'd be used, but turned out they aren't used yet.
Some of them just never got hooked up.

In any case, pushed, thanks!

Dan

> 
> Cheers,
> 
> Tom
> 
> 
> 
> 
> * add 4 new NMDeviceStateReason to map ModemManager errors more detailed
> * fix wrong error mapping for MM_MODEM_CONNECT_ERROR_NO_DIALTONE
> ---
>  include/NetworkManager.h         |   12 +++++++
>  src/modem-manager/nm-modem-gsm.c |   65 ++++++++++++++++++++++++++++---------
>  src/nm-device.c                  |    8 +++++
>  3 files changed, 69 insertions(+), 16 deletions(-)
> 
> diff --git a/include/NetworkManager.h b/include/NetworkManager.h
> index 4755f30..eb72564 100644
> --- a/include/NetworkManager.h
> +++ b/include/NetworkManager.h
> @@ -463,6 +463,18 @@ typedef enum {
>  	/* The Bluetooth connection failed or timed out */
>  	NM_DEVICE_STATE_REASON_BT_FAILED = 44,
>  
> +	/* GSM Modem's SIM Card not inserted */
> +	NM_DEVICE_STATE_REASON_GSM_SIM_NOT_INSERTED = 45,
> +
> +	/* GSM Modem's SIM Pin required */
> +	NM_DEVICE_STATE_REASON_GSM_SIM_PIN_REQUIRED = 46,
> +
> +	/* GSM Modem's SIM Puk required */
> +	NM_DEVICE_STATE_REASON_GSM_SIM_PUK_REQUIRED = 47,
> +
> +	/* GSM Modem's SIM wrong */
> +	NM_DEVICE_STATE_REASON_GSM_SIM_WRONG = 48,
> +
>  	/* Unused */
>  	NM_DEVICE_STATE_REASON_LAST = 0xFFFF
>  } NMDeviceStateReason;
> diff --git a/src/modem-manager/nm-modem-gsm.c b/src/modem-manager/nm-modem-gsm.c
> index 9a96ae9..777b2e2 100644
> --- a/src/modem-manager/nm-modem-gsm.c
> +++ b/src/modem-manager/nm-modem-gsm.c
> @@ -140,20 +140,34 @@ translate_mm_error (GError *error)
>  
>  	if (dbus_g_error_has_name (error, MM_MODEM_CONNECT_ERROR_NO_CARRIER))
>  		reason = NM_DEVICE_STATE_REASON_MODEM_NO_CARRIER;
> -	if (dbus_g_error_has_name (error, MM_MODEM_CONNECT_ERROR_NO_DIALTONE))
> -		reason = NM_DEVICE_STATE_REASON_MODEM_DIAL_TIMEOUT;
> -	if (dbus_g_error_has_name (error, MM_MODEM_CONNECT_ERROR_BUSY))
> +	else if (dbus_g_error_has_name (error, MM_MODEM_CONNECT_ERROR_NO_DIALTONE))
> +		reason = NM_DEVICE_STATE_REASON_MODEM_NO_DIAL_TONE;
> +	else if (dbus_g_error_has_name (error, MM_MODEM_CONNECT_ERROR_BUSY))
>  		reason = NM_DEVICE_STATE_REASON_MODEM_BUSY;
> -	if (dbus_g_error_has_name (error, MM_MODEM_CONNECT_ERROR_NO_ANSWER))
> +	else if (dbus_g_error_has_name (error, MM_MODEM_CONNECT_ERROR_NO_ANSWER))
>  		reason = NM_DEVICE_STATE_REASON_MODEM_DIAL_TIMEOUT;
> -	if (dbus_g_error_has_name (error, MM_MODEM_ERROR_NETWORK_NOT_ALLOWED))
> +	else if (dbus_g_error_has_name (error, MM_MODEM_ERROR_NETWORK_NOT_ALLOWED))
>  		reason = NM_DEVICE_STATE_REASON_GSM_REGISTRATION_DENIED;
> -	if (dbus_g_error_has_name (error, MM_MODEM_ERROR_NETWORK_TIMEOUT))
> +	else if (dbus_g_error_has_name (error, MM_MODEM_ERROR_NETWORK_TIMEOUT))
>  		reason = NM_DEVICE_STATE_REASON_GSM_REGISTRATION_TIMEOUT;
> -	if (dbus_g_error_has_name (error, MM_MODEM_ERROR_NO_NETWORK))
> +	else if (dbus_g_error_has_name (error, MM_MODEM_ERROR_NO_NETWORK))
>  		reason = NM_DEVICE_STATE_REASON_GSM_REGISTRATION_NOT_SEARCHING;
> +	else if (dbus_g_error_has_name (error, MM_MODEM_ERROR_SIM_NOT_INSERTED))
> +		reason = NM_DEVICE_STATE_REASON_GSM_SIM_NOT_INSERTED;
> +	else if (dbus_g_error_has_name (error, MM_MODEM_ERROR_SIM_PIN))
> +		reason = NM_DEVICE_STATE_REASON_GSM_SIM_PIN_REQUIRED;
> +	else if (dbus_g_error_has_name (error, MM_MODEM_ERROR_SIM_PUK))
> +		reason = NM_DEVICE_STATE_REASON_GSM_SIM_PUK_REQUIRED;
> +	else if (dbus_g_error_has_name (error, MM_MODEM_ERROR_SIM_WRONG))
> +		reason = NM_DEVICE_STATE_REASON_GSM_SIM_WRONG;
> +	else
> +	{
> +		/* unable to map the ModemManager error to a NM_DEVICE_STATE_REASON */
> +		nm_log_dbg (LOGD_MB, "unmapped dbus error detected: '%s'", dbus_g_error_get_name (error));
> +		reason = NM_DEVICE_STATE_REASON_UNKNOWN;
> +	}
>  
> -	/* FIXME: We have only GSM error messages here, and we have no idea which 
> +	/* FIXME: We have only GSM error messages here, and we have no idea which
>  	   activation state failed. Reasons like:
>  	   NM_DEVICE_STATE_REASON_MODEM_DIAL_FAILED,
>  	   NM_DEVICE_STATE_REASON_MODEM_INIT_FAILED,
> @@ -162,9 +176,6 @@ translate_mm_error (GError *error)
>  	   NM_DEVICE_STATE_REASON_GSM_PIN_CHECK_FAILED
>  	   are not used.
>  	*/
> -	else
> -		reason = NM_DEVICE_STATE_REASON_UNKNOWN;
> -
>  	return reason;
>  }
>  
> @@ -257,6 +268,7 @@ static void
>  stage1_pin_done (DBusGProxy *proxy, DBusGProxyCall *call_id, gpointer user_data)
>  {
>  	NMModemGsm *self = NM_MODEM_GSM (user_data);
> +	NMDeviceStateReason reason;
>  	GError *error = NULL;
>  
>  	if (dbus_g_proxy_end_call (proxy, call_id, &error, G_TYPE_INVALID)) {
> @@ -266,9 +278,19 @@ stage1_pin_done (DBusGProxy *proxy, DBusGProxyCall *call_id, gpointer user_data)
>  		nm_log_warn (LOGD_MB, "GSM PIN unlock failed: (%d) %s",
>  		             error ? error->code : -1,
>  		             error && error->message ? error->message : "(unknown)");
> -		g_error_free (error);
>  
> -		g_signal_emit_by_name (self, NM_MODEM_PREPARE_RESULT, FALSE, NM_DEVICE_STATE_REASON_MODEM_INIT_FAILED);
> +		/* try to translate the error reason */
> +		reason = translate_mm_error (error);
> +
> +		if (reason == NM_DEVICE_STATE_REASON_UNKNOWN) {
> +			/* use 'generic' reason NM_DEVICE_STATE_REASON_MODEM_INIT_FAILED */
> +			g_signal_emit_by_name (self, NM_MODEM_PREPARE_RESULT, FALSE, NM_DEVICE_STATE_REASON_MODEM_INIT_FAILED);
> +		} else {
> +			/* use translated reason */
> +			g_signal_emit_by_name (self, NM_MODEM_PREPARE_RESULT, FALSE, reason);
> +		}
> +
> +		g_error_free (error);
>  	}
>  }
>  
> @@ -301,6 +323,7 @@ static void
>  stage1_enable_done (DBusGProxy *proxy, DBusGProxyCall *call_id, gpointer user_data)
>  {
>  	NMModemGsm *self = NM_MODEM_GSM (user_data);
> +	NMDeviceStateReason reason;
>  	GError *error = NULL;
>  
>  	if (dbus_g_proxy_end_call (proxy, call_id, &error, G_TYPE_INVALID))
> @@ -310,10 +333,20 @@ stage1_enable_done (DBusGProxy *proxy, DBusGProxyCall *call_id, gpointer user_da
>  		             error ? error->code : -1,
>  		             error && error->message ? error->message : "(unknown)");
>  
> -		if (dbus_g_error_has_name (error, MM_MODEM_ERROR_SIM_PIN))
> +		if (dbus_g_error_has_name (error, MM_MODEM_ERROR_SIM_PIN)) {
>  			handle_enable_pin_required (self);
> -		else
> -			g_signal_emit_by_name (self, NM_MODEM_PREPARE_RESULT, FALSE, NM_DEVICE_STATE_REASON_MODEM_INIT_FAILED);
> +		} else {
> +			/* try to translate the error reason */
> +			reason = translate_mm_error (error);
> +
> +			if (reason == NM_DEVICE_STATE_REASON_UNKNOWN) {
> +				/* use 'generic' reason NM_DEVICE_STATE_REASON_MODEM_INIT_FAILED */
> +				g_signal_emit_by_name (self, NM_MODEM_PREPARE_RESULT, FALSE, NM_DEVICE_STATE_REASON_MODEM_INIT_FAILED);
> +			} else {
> +				/* use translated reason */
> +				g_signal_emit_by_name (self, NM_MODEM_PREPARE_RESULT, FALSE, reason);
> +			}
> +		}
>  
>  		g_error_free (error);
>  	}
> diff --git a/src/nm-device.c b/src/nm-device.c
> index e285a57..53eece8 100644
> --- a/src/nm-device.c
> +++ b/src/nm-device.c
> @@ -3863,6 +3863,14 @@ reason_to_string (NMDeviceStateReason reason)
>  		return "modem-not-found";
>  	case NM_DEVICE_STATE_REASON_BT_FAILED:
>  		return "bluetooth-failed";
> +	case NM_DEVICE_STATE_REASON_GSM_SIM_NOT_INSERTED:
> +		return "gsm-sim-not-inserted";
> +	case NM_DEVICE_STATE_REASON_GSM_SIM_PIN_REQUIRED:
> +		return "gsm-sim-pin-required";
> +	case NM_DEVICE_STATE_REASON_GSM_SIM_PUK_REQUIRED:
> +		return "gsm-sim-puk-required";
> +	case NM_DEVICE_STATE_REASON_GSM_SIM_WRONG:
> +		return "gsm-sim-wrong";
>  	default:
>  		break;
>  	}




[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]