From: Mikel Astiz <[email protected]>
This third version fixes a mistake in the previous proposal, as pointed out by Johan.
The rest remains the same and has been acked by Luiz.
Mikel Astiz (6):
client: fix memory leak in obc_session_put
client: fix unreported error case
client: fix incorrect error check
client: fix pbap select when same path given twice
client: queue transfers in ftp sessions
client: queue transfers in pbap sessions
client/ftp.c | 39 ++++---------
client/pbap.c | 176 ++++++++++++++++++++++++------------------------------
client/session.c | 22 ++++++-
3 files changed, 109 insertions(+), 128 deletions(-)
--
1.7.6.5
Hi Mikel,
On Thu, Feb 16, 2012, Mikel Astiz wrote:
> +struct pending_request *pending_request_new(struct pbap_data *pbap,
> + DBusMessage *message)
> +{
> + struct pending_request *p;
> +
> + p = g_new0(struct pending_request, 1);
> + p->pbap = pbap;
> + p->msg = dbus_message_ref(message);
> +
> + return p;
> +}
This will cause a compilation warning/error since you're not exporting
the function through any .h file. I suppose you intended to declare it
static? Also, this implies that you're not doing compilation tests with
./bootstrap-configure since it converts all warnings to errors and would
have made it impossible to compile the code. Please always use that when
writing new code.
Johan
Hi Mikel,
On Thu, Feb 16, 2012, Mikel Astiz wrote:
> - obc_session_setpath(pbap->session, path, pbap_setpath_cb, pbap, err);
> - if (err != NULL) {
> + if (obc_session_setpath(pbap->session, path, pbap_setpath_cb, pbap,
> + err)) {
I know it adds a little bit of extra code, but I think it'd make this a
bit more readable if you added a new "guint id" variable and do:
id = obc_session_setpath(...);
if (id > 0) {
...
}
Johan
Hi Mikel,
On Thu, Feb 16, 2012, Mikel Astiz wrote:
> @@ -735,17 +735,31 @@ static void session_process_queue(struct obc_session *session)
> if (session->queue == NULL || g_queue_is_empty(session->queue))
> return;
>
> + obc_session_ref(session);
> +
> while ((p = g_queue_pop_head(session->queue))) {
> int err;
>
> err = pending_request_auth(p);
> if (err == 0) {
> session->p = p;
> + obc_session_unref(session);
> return;
> }
Instead of adding the unref to any place you return in the loop, it'd be
simpler to just do a break; since you've already got the necessary unref
and end of the function right after the loop.
Johan
From: Mikel Astiz <[email protected]>
Previous implementation reported busy when trying to queue several
operations in the same session.
---
client/pbap.c | 127 +++++++++++++++++++++++++++++---------------------------
1 files changed, 66 insertions(+), 61 deletions(-)
diff --git a/client/pbap.c b/client/pbap.c
index c4b14a2..eed7fa6 100644
--- a/client/pbap.c
+++ b/client/pbap.c
@@ -127,12 +127,16 @@ static const char *filter_list[] = {
struct pbap_data {
struct obc_session *session;
char *path;
- DBusMessage *msg;
guint8 format;
guint8 order;
uint64_t filter;
};
+struct pending_request {
+ struct pbap_data *pbap;
+ DBusMessage *msg;
+};
+
struct pullphonebook_apparam {
uint8_t filter_tag;
uint8_t filter_len;
@@ -167,6 +171,24 @@ struct apparam_hdr {
static DBusConnection *conn = NULL;
+struct pending_request *pending_request_new(struct pbap_data *pbap,
+ DBusMessage *message)
+{
+ struct pending_request *p;
+
+ p = g_new0(struct pending_request, 1);
+ p->pbap = pbap;
+ p->msg = dbus_message_ref(message);
+
+ return p;
+}
+
+static void pending_request_free(struct pending_request *p)
+{
+ dbus_message_unref(p->msg);
+ g_free(p);
+}
+
static void listing_element(GMarkupParseContext *ctxt,
const gchar *element,
const gchar **names,
@@ -252,24 +274,21 @@ static void pbap_reset_path(struct pbap_data *pbap)
static void pbap_setpath_cb(struct obc_session *session, GError *err,
gpointer user_data)
{
- struct pbap_data *pbap = user_data;
+ struct pending_request *request = user_data;
+ struct pbap_data *pbap = request->pbap;
if (err != NULL)
- pbap_reset_path(user_data);
-
- if (pbap->msg == NULL)
- return;
+ pbap_reset_path(pbap);
if (err) {
- DBusMessage *reply= g_dbus_create_error(pbap->msg,
+ DBusMessage *reply = g_dbus_create_error(request->msg,
ERROR_INF ".Failed",
"%s", err->message);
g_dbus_send_message(conn, reply);
} else
- g_dbus_send_reply(conn, pbap->msg, DBUS_TYPE_INVALID);
+ g_dbus_send_reply(conn, request->msg, DBUS_TYPE_INVALID);
- dbus_message_unref(pbap->msg);
- pbap->msg = NULL;
+ pending_request_free(request);
}
static void read_return_apparam(struct obc_session *session,
@@ -322,22 +341,19 @@ static void read_return_apparam(struct obc_session *session,
static void pull_phonebook_callback(struct obc_session *session,
GError *err, void *user_data)
{
- struct pbap_data *pbap = user_data;
+ struct pending_request *request = user_data;
DBusMessage *reply;
const char *buf;
size_t size;
- if (pbap->msg == NULL)
- return;
-
if (err) {
- reply = g_dbus_create_error(pbap->msg,
+ reply = g_dbus_create_error(request->msg,
"org.openobex.Error.Failed",
"%s", err->message);
goto send;
}
- reply = dbus_message_new_method_return(pbap->msg);
+ reply = dbus_message_new_method_return(request->msg);
buf = obc_session_get_buffer(session, &size);
if (size == 0)
@@ -349,29 +365,25 @@ static void pull_phonebook_callback(struct obc_session *session,
send:
g_dbus_send_message(conn, reply);
- dbus_message_unref(pbap->msg);
- pbap->msg = NULL;
+ pending_request_free(request);
}
static void phonebook_size_callback(struct obc_session *session,
GError *err, void *user_data)
{
- struct pbap_data *pbap = user_data;
+ struct pending_request *request = user_data;
DBusMessage *reply;
guint16 phone_book_size;
guint8 new_missed_calls;
- if (pbap->msg == NULL)
- return;
-
if (err) {
- reply = g_dbus_create_error(pbap->msg,
+ reply = g_dbus_create_error(request->msg,
"org.openobex.Error.Failed",
"%s", err->message);
goto send;
}
- reply = dbus_message_new_method_return(pbap->msg);
+ reply = dbus_message_new_method_return(request->msg);
read_return_apparam(session, &phone_book_size, &new_missed_calls);
@@ -381,31 +393,27 @@ static void phonebook_size_callback(struct obc_session *session,
send:
g_dbus_send_message(conn, reply);
- dbus_message_unref(pbap->msg);
- pbap->msg = NULL;
+ pending_request_free(request);
}
static void pull_vcard_listing_callback(struct obc_session *session,
GError *err, void *user_data)
{
- struct pbap_data *pbap = user_data;
+ struct pending_request *request = user_data;
GMarkupParseContext *ctxt;
DBusMessage *reply;
DBusMessageIter iter, array;
const char *buf;
size_t size;
- if (pbap->msg == NULL)
- return;
-
if (err) {
- reply = g_dbus_create_error(pbap->msg,
+ reply = g_dbus_create_error(request->msg,
"org.openobex.Error.Failed",
"%s", err->message);
goto send;
}
- reply = dbus_message_new_method_return(pbap->msg);
+ reply = dbus_message_new_method_return(request->msg);
buf = obc_session_get_buffer(session, &size);
if (size == 0)
@@ -423,8 +431,7 @@ static void pull_vcard_listing_callback(struct obc_session *session,
send:
g_dbus_send_message(conn, reply);
- dbus_message_unref(pbap->msg);
- pbap->msg = NULL;
+ pending_request_free(request);
}
static DBusMessage *pull_phonebook(struct pbap_data *pbap,
@@ -433,14 +440,10 @@ static DBusMessage *pull_phonebook(struct pbap_data *pbap,
guint8 format, guint16 maxlistcount,
guint16 liststartoffset)
{
+ struct pending_request *request;
struct pullphonebook_apparam apparam;
session_callback_t func;
- if (pbap->msg)
- return g_dbus_create_error(message,
- "org.openobex.Error.InProgress",
- "Transfer in progress");
-
apparam.filter_tag = FILTER_TAG;
apparam.filter_len = FILTER_LEN;
apparam.filter = GUINT64_TO_BE(filter);
@@ -466,14 +469,16 @@ static DBusMessage *pull_phonebook(struct pbap_data *pbap,
return NULL;
}
+ request = pending_request_new(pbap, message);
+
if (obc_session_get(pbap->session, "x-bt/phonebook", name, NULL,
(guint8 *) &apparam, sizeof(apparam),
- func, pbap) < 0)
+ func, request) < 0) {
+ pending_request_free(request);
return g_dbus_create_error(message,
"org.openobex.Error.Failed",
"Failed");
-
- pbap->msg = dbus_message_ref(message);
+ }
return NULL;
}
@@ -495,15 +500,11 @@ static DBusMessage *pull_vcard_listing(struct pbap_data *pbap,
guint8 order, char *searchval, guint8 attrib,
guint16 count, guint16 offset)
{
+ struct pending_request *request;
guint8 *p, *apparam = NULL;
gint apparam_size;
int err;
- if (pbap->msg)
- return g_dbus_create_error(message,
- "org.openobex.Error.InProgress",
- "Transfer in progress");
-
/* trunc the searchval string if it's length exceed the max value of guint8 */
if (strlen(searchval) > 254)
searchval[255] = '\0';
@@ -530,16 +531,18 @@ static DBusMessage *pull_vcard_listing(struct pbap_data *pbap,
offset = GUINT16_TO_BE(offset);
p = fill_apparam(p, &offset, LISTSTARTOFFSET_TAG, LISTSTARTOFFSET_LEN);
+ request = pending_request_new(pbap, message);
+
err = obc_session_get(pbap->session, "x-bt/vcard-listing", name, NULL,
apparam, apparam_size,
- pull_vcard_listing_callback, pbap);
+ pull_vcard_listing_callback, request);
g_free(apparam);
- if (err < 0)
+ if (err < 0) {
+ pending_request_free(request);
return g_dbus_create_error(message,
"org.openobex.Error.Failed",
"Failed");
-
- pbap->msg = dbus_message_ref(message);
+ }
return NULL;
}
@@ -663,6 +666,7 @@ static DBusMessage *pbap_select(DBusConnection *connection,
struct pbap_data *pbap = user_data;
const char *item, *location;
char *path;
+ struct pending_request *request;
GError *err = NULL;
if (dbus_message_get_args(message, NULL,
@@ -682,19 +686,22 @@ static DBusMessage *pbap_select(DBusConnection *connection,
return dbus_message_new_method_return(message);
}
- obc_session_setpath(pbap->session, path, pbap_setpath_cb, pbap, &err);
+ request = pending_request_new(pbap, message);
+
+ obc_session_setpath(pbap->session, path, pbap_setpath_cb, request,
+ &err);
if (err != NULL) {
DBusMessage *reply;
reply = g_dbus_create_error(message, ERROR_INF ".Failed",
"%s", err->message);
g_error_free(err);
g_free(path);
+ pending_request_free(request);
return reply;
}
g_free(pbap->path);
pbap->path = path;
- pbap->msg = dbus_message_ref(message);
return NULL;
}
@@ -725,6 +732,7 @@ static DBusMessage *pbap_pull_vcard(DBusConnection *connection,
struct pbap_data *pbap = user_data;
struct pullvcardentry_apparam apparam;
const char *name;
+ struct pending_request *request;
if (!pbap->path)
return g_dbus_create_error(message,
@@ -737,11 +745,6 @@ static DBusMessage *pbap_pull_vcard(DBusConnection *connection,
return g_dbus_create_error(message,
ERROR_INF ".InvalidArguments", NULL);
- if (pbap->msg)
- return g_dbus_create_error(message,
- "org.openobex.Error.InProgress",
- "Transfer in progress");
-
apparam.filter_tag = FILTER_TAG;
apparam.filter_len = FILTER_LEN;
apparam.filter = GUINT64_TO_BE(pbap->filter);
@@ -749,14 +752,16 @@ static DBusMessage *pbap_pull_vcard(DBusConnection *connection,
apparam.format_len = FORMAT_LEN;
apparam.format = pbap->format;
+ request = pending_request_new(pbap, message);
+
if (obc_session_get(pbap->session, "x-bt/vcard", name, NULL,
(guint8 *)&apparam, sizeof(apparam),
- pull_phonebook_callback, pbap) < 0)
+ pull_phonebook_callback, request) < 0) {
+ pending_request_free(request);
return g_dbus_create_error(message,
"org.openobex.Error.Failed",
"Failed");
-
- pbap->msg = dbus_message_ref(message);
+ }
return NULL;
}
--
1.7.6.5
From: Mikel Astiz <[email protected]>
If the same path is selected twice, the operation can be skipped but the
D-Bus response should still be sent.
---
client/pbap.c | 57 ++++++++++++++++-----------------------------------------
1 files changed, 16 insertions(+), 41 deletions(-)
diff --git a/client/pbap.c b/client/pbap.c
index 4bb7a70..c4b14a2 100644
--- a/client/pbap.c
+++ b/client/pbap.c
@@ -124,12 +124,6 @@ static const char *filter_list[] = {
#define PBAP_INTERFACE "org.openobex.PhonebookAccess"
#define PBAP_UUID "0000112f-0000-1000-8000-00805f9b34fb"
-#define PBAP_ERROR pbap_error_quark()
-
-enum {
- PBAP_INVALID_PATH,
-};
-
struct pbap_data {
struct obc_session *session;
char *path;
@@ -173,11 +167,6 @@ struct apparam_hdr {
static DBusConnection *conn = NULL;
-static GQuark pbap_error_quark(void)
-{
- return g_quark_from_static_string("pbap-error-quark");
-}
-
static void listing_element(GMarkupParseContext *ctxt,
const gchar *element,
const gchar **names,
@@ -283,35 +272,6 @@ static void pbap_setpath_cb(struct obc_session *session, GError *err,
pbap->msg = NULL;
}
-static gboolean pbap_setpath(struct pbap_data *pbap, const char *location,
- const char *item, GError **err)
-{
- char *path;
-
- path = build_phonebook_path(location, item);
- if (path == NULL) {
- g_set_error(err, PBAP_ERROR, PBAP_INVALID_PATH,
- "Invalid path");
- return FALSE;
- }
-
- if (pbap->path != NULL && g_str_equal(pbap->path, path)) {
- g_free(path);
- return TRUE;
- }
-
- if (obc_session_setpath(pbap->session, path, pbap_setpath_cb, pbap,
- err)) {
- g_free(pbap->path);
- pbap->path = path;
- return TRUE;
- }
-
- g_free(path);
-
- return FALSE;
-}
-
static void read_return_apparam(struct obc_session *session,
guint16 *phone_book_size, guint8 *new_missed_calls)
{
@@ -702,6 +662,7 @@ static DBusMessage *pbap_select(DBusConnection *connection,
{
struct pbap_data *pbap = user_data;
const char *item, *location;
+ char *path;
GError *err = NULL;
if (dbus_message_get_args(message, NULL,
@@ -711,14 +672,28 @@ static DBusMessage *pbap_select(DBusConnection *connection,
return g_dbus_create_error(message,
ERROR_INF ".InvalidArguments", NULL);
- if (!pbap_setpath(pbap, location, item, &err)) {
+ path = build_phonebook_path(location, item);
+ if (path == NULL)
+ return g_dbus_create_error(message,
+ ERROR_INF ".InvalidArguments", "Invalid path");
+
+ if (pbap->path != NULL && g_str_equal(pbap->path, path)) {
+ g_free(path);
+ return dbus_message_new_method_return(message);
+ }
+
+ obc_session_setpath(pbap->session, path, pbap_setpath_cb, pbap, &err);
+ if (err != NULL) {
DBusMessage *reply;
reply = g_dbus_create_error(message, ERROR_INF ".Failed",
"%s", err->message);
g_error_free(err);
+ g_free(path);
return reply;
}
+ g_free(pbap->path);
+ pbap->path = path;
pbap->msg = dbus_message_ref(message);
return NULL;
--
1.7.6.5
From: Mikel Astiz <[email protected]>
Previous implementation reported busy when trying to queue several
transfers in the same session.
---
client/ftp.c | 39 +++++++++++----------------------------
1 files changed, 11 insertions(+), 28 deletions(-)
diff --git a/client/ftp.c b/client/ftp.c
index 9e3f6b3..9be5d69 100644
--- a/client/ftp.c
+++ b/client/ftp.c
@@ -48,7 +48,6 @@ static DBusConnection *conn = NULL;
struct ftp_data {
struct obc_session *session;
- DBusMessage *msg;
};
static void async_cb(struct obc_session *session, GError *err,
@@ -176,36 +175,31 @@ static const GMarkupParser parser = {
static void get_file_callback(struct obc_session *session, GError *err,
void *user_data)
{
- struct ftp_data *ftp = user_data;
+ DBusMessage *msg = user_data;
DBusMessage *reply;
- if (!ftp->msg)
- return;
-
if (err)
- reply = g_dbus_create_error(ftp->msg,
+ reply = g_dbus_create_error(msg,
"org.openobex.Error.Failed",
"%s", err->message);
else
- reply = dbus_message_new_method_return(ftp->msg);
+ reply = dbus_message_new_method_return(msg);
g_dbus_send_message(conn, reply);
-
- dbus_message_unref(ftp->msg);
- ftp->msg = NULL;
+ dbus_message_unref(msg);
}
static void list_folder_callback(struct obc_session *session,
GError *err, void *user_data)
{
- struct ftp_data *ftp = user_data;
+ DBusMessage *msg = user_data;
GMarkupParseContext *ctxt;
DBusMessage *reply;
DBusMessageIter iter, array;
const char *buf;
size_t size;
- reply = dbus_message_new_method_return(ftp->msg);
+ reply = dbus_message_new_method_return(msg);
buf = obc_session_get_buffer(session, &size);
if (size == 0)
@@ -224,8 +218,7 @@ static void list_folder_callback(struct obc_session *session,
done:
g_dbus_send_message(conn, reply);
- dbus_message_unref(ftp->msg);
- ftp->msg = NULL;
+ dbus_message_unref(msg);
}
static DBusMessage *create_folder(DBusConnection *connection,
@@ -263,18 +256,13 @@ static DBusMessage *list_folder(DBusConnection *connection,
struct ftp_data *ftp = user_data;
struct obc_session *session = ftp->session;
- if (ftp->msg)
- return g_dbus_create_error(message,
- "org.openobex.Error.InProgress",
- "Transfer in progress");
-
if (obc_session_get(session, "x-obex/folder-listing",
- NULL, NULL, NULL, 0, list_folder_callback, ftp) < 0)
+ NULL, NULL, NULL, 0, list_folder_callback, message) < 0)
return g_dbus_create_error(message,
"org.openobex.Error.Failed",
"Failed");
- ftp->msg = dbus_message_ref(message);
+ dbus_message_ref(message);
return NULL;
}
@@ -286,11 +274,6 @@ static DBusMessage *get_file(DBusConnection *connection,
struct obc_session *session = ftp->session;
const char *target_file, *source_file;
- if (ftp->msg)
- return g_dbus_create_error(message,
- "org.openobex.Error.InProgress",
- "Transfer in progress");
-
if (dbus_message_get_args(message, NULL,
DBUS_TYPE_STRING, &target_file,
DBUS_TYPE_STRING, &source_file,
@@ -299,12 +282,12 @@ static DBusMessage *get_file(DBusConnection *connection,
"org.openobex.Error.InvalidArguments", NULL);
if (obc_session_get(session, NULL, source_file,
- target_file, NULL, 0, get_file_callback, ftp) < 0)
+ target_file, NULL, 0, get_file_callback, message) < 0)
return g_dbus_create_error(message,
"org.openobex.Error.Failed",
"Failed");
- ftp->msg = dbus_message_ref(message);
+ dbus_message_ref(message);
return NULL;
}
--
1.7.6.5
From: Mikel Astiz <[email protected]>
Previous statement always returned success.
---
client/pbap.c | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/client/pbap.c b/client/pbap.c
index 4910536..4bb7a70 100644
--- a/client/pbap.c
+++ b/client/pbap.c
@@ -300,8 +300,8 @@ static gboolean pbap_setpath(struct pbap_data *pbap, const char *location,
return TRUE;
}
- obc_session_setpath(pbap->session, path, pbap_setpath_cb, pbap, err);
- if (err != NULL) {
+ if (obc_session_setpath(pbap->session, path, pbap_setpath_cb, pbap,
+ err)) {
g_free(pbap->path);
pbap->path = path;
return TRUE;
--
1.7.6.5
From: Mikel Astiz <[email protected]>
obc_session_put takes ownership of the given buffer, but did not free
the memory in case of error.
---
client/session.c | 8 ++++++--
1 files changed, 6 insertions(+), 2 deletions(-)
diff --git a/client/session.c b/client/session.c
index 0fa8efc..28516b2 100644
--- a/client/session.c
+++ b/client/session.c
@@ -1017,8 +1017,10 @@ int obc_session_put(struct obc_session *session, char *buf, const char *targetna
struct obc_transfer *transfer;
const char *agent;
- if (session->obex == NULL)
+ if (session->obex == NULL) {
+ g_free(buf);
return -ENOTCONN;
+ }
agent = obc_agent_get_name(session->agent);
@@ -1026,8 +1028,10 @@ int obc_session_put(struct obc_session *session, char *buf, const char *targetna
agent, NULL,
targetname, NULL,
NULL);
- if (transfer == NULL)
+ if (transfer == NULL) {
+ g_free(buf);
return -EIO;
+ }
obc_transfer_set_buffer(transfer, buf);
--
1.7.6.5
From: Mikel Astiz <[email protected]>
The authorization request of a queued transfer could fail, and this
needs to be reported to the transfer initiator. Otherwise it would
likely result in D-Bus timeouts.
---
client/session.c | 14 ++++++++++++++
1 files changed, 14 insertions(+), 0 deletions(-)
diff --git a/client/session.c b/client/session.c
index 28516b2..4d49fc4 100644
--- a/client/session.c
+++ b/client/session.c
@@ -735,17 +735,31 @@ static void session_process_queue(struct obc_session *session)
if (session->queue == NULL || g_queue_is_empty(session->queue))
return;
+ obc_session_ref(session);
+
while ((p = g_queue_pop_head(session->queue))) {
int err;
err = pending_request_auth(p);
if (err == 0) {
session->p = p;
+ obc_session_unref(session);
return;
}
+ if (p->func) {
+ GError *gerr = NULL;
+
+ g_set_error(&gerr, OBEX_IO_ERROR, err,
+ "Authorization failed");
+ p->func(session, gerr, p->data);
+ g_error_free(gerr);
+ }
+
pending_request_free(p);
}
+
+ obc_session_unref(session);
}
static void session_terminate_transfer(struct obc_session *session,
--
1.7.6.5