Hi Frédéric,

On 05/20/2011 04:40 AM, Frédéric Danis wrote:
> split manager_dial between generic and dbus parts
> ---
>  src/voicecall.c |   91 ++++++++++++++++++++++++++++++++++++++----------------
>  1 files changed, 64 insertions(+), 27 deletions(-)
> 

<snip>

> +
> +static DBusMessage *manager_dial(DBusConnection *conn,
> +					DBusMessage *msg, void *data)
> +{
> +	struct ofono_voicecall *vc = data;
> +	const char *number;
> +	const char *clirstr;
> +	enum ofono_clir_option clir;
> +	int err;
> +	DBusMessage *reply = NULL;
> +
> +	if (vc->pending)
> +		return __ofono_error_busy(msg);
> +
> +	if (dbus_message_get_args(msg, NULL, DBUS_TYPE_STRING, &number,
> +					DBUS_TYPE_STRING, &clirstr,
> +					DBUS_TYPE_INVALID) == FALSE)
> +		return __ofono_error_invalid_args(msg);
> +
> +	if (clir_string_to_clir(clirstr, &clir) == FALSE)
> +		return __ofono_error_invalid_format(msg);
> +
>  	vc->pending = dbus_message_ref(msg);
>  
> -	string_to_phone_number(number, &ph);
> +	err = voicecall_dial(vc, number, clir, manager_dial_callback, vc);
> +	switch (err) {
> +	case 0:
> +		break;

Maybe I'm missing something, but why don't you do something like:

if (err >= 0)
	return NULL;

vc->pending = NULL;
dbus_message_unref(msg);

> +
> +	case -EINVAL:
> +		reply = __ofono_error_invalid_format(msg);
> +		break;

And then simply use return __ofono_error... here?

>  
> -	vc->driver->dial(vc, &ph, clir, manager_dial_callback, vc);
> +	case -ENETDOWN:
> +		reply = __ofono_error_not_available(msg);
> +		break;
>  

And here

> -	return NULL;
> +	case -ENOTSUP:
> +		reply = __ofono_error_not_implemented(msg);
> +		break;
> +

And here

> +	default:
> +		reply = __ofono_error_failed(msg);
> +		break;
> +	}
> +

And here

> +	if (reply != NULL) {
> +		dbus_message_unref(msg);
> +		vc->pending = NULL;
> +	}
> +
> +	return reply;

And then this part is not needed.

>  }
>  
>  static DBusMessage *manager_transfer(DBusConnection *conn,

Regards,
-Denis