Return-Path: <cyrus@holtmann.org>
MIME-Version: 1.0
In-Reply-To: <20090513064940.GA22585@jh-x301>
References: <1242183319-24470-1-git-send-email-forrest.zhao@intel.com>
	 <20090513064940.GA22585@jh-x301>
Date: Wed, 13 May 2009 15:02:53 +0800
Message-ID: <ac8af0be0905130002t401ed339i8f5575a1aa698bad@mail.gmail.com>
Subject: Re: [PATCH] add support for HFP plugin for oFono(ofono.org)
From: Zhao Forrest <forrest.zhao@gmail.com>
To: Forrest Zhao <forrest.zhao@intel.com>,
	linux-bluetooth@vger.kernel.org, forrest.zhao@gmail.com
Content-Type: text/plain; charset=ISO-8859-1
Sender: linux-bluetooth-owner@vger.kernel.org
List-ID: <linux-bluetooth.vger.kernel.org>

>
> On Wed, May 13, 2009, Forrest Zhao wrote:
>> +static int send_method_call(const char *dest, const char *path,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?const char *interface, const char *method,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?DBusPendingCallNotifyFunction cb,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?void *user_data, int type, ...);
>
> I don't think there should be any need for this forward declaration. Just
> move the function higher up in the file.
OK.

>> +static void answer_reply(DBusPendingCall *call, void *user_data)
>> +{
>> + ? ? DBusMessage *reply;
>> + ? ? DBusError err;
>> +
>> + ? ? reply = dbus_pending_call_steal_reply(call);
>> + ? ? dbus_error_init(&err);
>> + ? ? if (dbus_set_error_from_message(&err, reply)) {
>> + ? ? ? ? ? ? error("oFono answer incoming call failed: %s, %s",
>> + ? ? ? ? ? ? ? ? ? ? err.name, err.message);
>> + ? ? ? ? ? ? dbus_error_free(&err);
>> + ? ? ? ? ? ? goto done;
>> + ? ? }
>> +
>> +done:
>> + ? ? dbus_message_unref(reply);
>> +}
>> +
>> +void telephony_answer_call_req(void *telephony_device)
>> +{
>> + ? ? struct voice_call *vc = find_vc_with_status(CALL_STATUS_INCOMING);
>> + ? ? int ret;
>> +
>> + ? ? if (!vc) {
>> + ? ? ? ? ? ? telephony_answer_call_rsp(telephony_device,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? CME_ERROR_NOT_ALLOWED);
>> + ? ? ? ? ? ? return;
>> + ? ? }
>> +
>> + ? ? ret = send_method_call(OFONO_BUS_NAME, vc->obj_path,
>> + ? ? ? ? ? ? ? ? ? ? OFONO_VC_INTERFACE,
>> + ? ? ? ? ? ? ? ? ? ? "Answer", answer_reply,
>> + ? ? ? ? ? ? ? ? ? ? NULL, DBUS_TYPE_INVALID);
>> +
>> + ? ? if (ret < 0) {
>> + ? ? ? ? ? ? telephony_answer_call_rsp(telephony_device,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? CME_ERROR_AG_FAILURE);
>> + ? ? ? ? ? ? return;
>> + ? ? }
>> +
>> + ? ? telephony_answer_call_rsp(telephony_device, CME_ERROR_NONE);
>> +}
>
> If you're actually going to handle the D-Bus reply to the Answer method
> call (which the maemo driver doesn't do -- it relies on the state signals
> instead), doesn't it make sense to call telephony_answer_call_rsp in the
> reply handler (answer_reply) instead of here?
After reviewing the patch I think I should remove answer_reply() because
it also depends on the state signals to know if "answer" actually succeed.

Thanks,
Forrest