2011-05-31 10:46:47

by Rymarkiewicz Waldemar

[permalink] [raw]
Subject: [PATCH v3 1/4] Clean SAP server error printouts

---
sap/server.c | 75 ++++++++++++++++++++++++----------------------------------
1 files changed, 31 insertions(+), 44 deletions(-)

diff --git a/sap/server.c b/sap/server.c
index 56a62f7..c5ea97c 100644
--- a/sap/server.c
+++ b/sap/server.c
@@ -338,8 +338,6 @@ static void connect_req(struct sap_connection *conn,
return;

error_rsp:
- error("Processing error (param %p state %d pr 0x%02x)", param,
- conn->state, conn->processing_req);
sap_error_rsp(conn);
}

@@ -387,8 +385,10 @@ static int disconnect_req(struct sap_connection *conn, uint8_t disc_type)

case SAP_DISCONNECTION_TYPE_CLIENT:
if (conn->state != SAP_STATE_CONNECTED &&
- conn->state != SAP_STATE_GRACEFUL_DISCONNECT)
- goto error_rsp;
+ conn->state != SAP_STATE_GRACEFUL_DISCONNECT) {
+ sap_error_rsp(conn);
+ goto error_req;
+ }

conn->state = SAP_STATE_CLIENT_DISCONNECT;
conn->processing_req = SAP_NO_REQ;
@@ -403,11 +403,7 @@ static int disconnect_req(struct sap_connection *conn, uint8_t disc_type)
return -EINVAL;
}

-error_rsp:
- sap_error_rsp(conn);
error_req:
- error("Processing error (state %d pr 0x%02x)", conn->state,
- conn->processing_req);
return -EPERM;
}

@@ -434,8 +430,6 @@ static void transfer_apdu_req(struct sap_connection *conn,
return;

error_rsp:
- error("Processing error (param %p state %d pr 0x%02x)", param,
- conn->state, conn->processing_req);
sap_error_rsp(conn);
}

@@ -455,8 +449,6 @@ static void transfer_atr_req(struct sap_connection *conn)
return;

error_rsp:
- error("Processing error (state %d pr 0x%02x)", conn->state,
- conn->processing_req);
sap_error_rsp(conn);
}

@@ -476,8 +468,6 @@ static void power_sim_off_req(struct sap_connection *conn)
return;

error_rsp:
- error("Processing error (state %d pr 0x%02x)", conn->state,
- conn->processing_req);
sap_error_rsp(conn);
}

@@ -497,8 +487,6 @@ static void power_sim_on_req(struct sap_connection *conn)
return;

error_rsp:
- error("Processing error (state %d pr 0x%02x)", conn->state,
- conn->processing_req);
sap_error_rsp(conn);
}

@@ -518,8 +506,6 @@ static void reset_sim_req(struct sap_connection *conn)
return;

error_rsp:
- error("Processing error (state %d pr 0x%02x param)", conn->state,
- conn->processing_req);
sap_error_rsp(conn);
}

@@ -539,8 +525,6 @@ static void transfer_card_reader_status_req(struct sap_connection *conn)
return;

error_rsp:
- error("Processing error (state %d pr 0x%02x)", conn->state,
- conn->processing_req);
sap_error_rsp(conn);
}

@@ -564,8 +548,6 @@ static void set_transport_protocol_req(struct sap_connection *conn,
return;

error_rsp:
- error("Processing error (param %p state %d pr 0x%02x)", param,
- conn->state, conn->processing_req);
sap_error_rsp(conn);
}

@@ -967,6 +949,9 @@ int sap_error_rsp(void *sap_device)
memset(&msg, 0, sizeof(msg));
msg.id = SAP_ERROR_RESP;

+ error("SAP error (state %d pr 0x%02x).", conn->state,
+ conn->processing_req);
+
return send_message(conn, &msg, sizeof(msg));
}

@@ -1008,10 +993,9 @@ int sap_disconnect_ind(void *sap_device, uint8_t disc_type)
return disconnect_req(conn, SAP_DISCONNECTION_TYPE_IMMEDIATE);
}

-static int handle_cmd(void *data, void *buf, size_t size)
+static int handle_cmd(struct sap_connection *conn, void *buf, size_t size)
{
struct sap_message *msg = buf;
- struct sap_connection *conn = data;

if (!conn)
return -EINVAL;
@@ -1055,12 +1039,12 @@ static int handle_cmd(void *data, void *buf, size_t size)
set_transport_protocol_req(conn, msg->param);
return 0;
default:
- DBG("SAP unknown message.");
+ DBG("Unknown SAP message id 0x%02x.", msg->id);
break;
}

error_rsp:
- DBG("Bad request message format.");
+ DBG("Invalid SAP message format.");
sap_error_rsp(conn);
return -EBADMSG;
}
@@ -1084,12 +1068,14 @@ static void sap_conn_remove(struct sap_connection *conn)

static gboolean sap_io_cb(GIOChannel *io, GIOCondition cond, gpointer data)
{
+ struct sap_connection *conn = data;
+
char buf[SAP_BUF_SIZE];
size_t bytes_read = 0;
GError *gerr = NULL;
GIOStatus gstatus;

- DBG("io %p", io);
+ DBG("conn %p io %p", conn, io);

if (cond & G_IO_NVAL) {
DBG("ERR (G_IO_NVAL) on rfcomm socket.");
@@ -1115,8 +1101,8 @@ static gboolean sap_io_cb(GIOChannel *io, GIOCondition cond, gpointer data)
return TRUE;
}

- if (handle_cmd(data, buf, bytes_read) < 0)
- error("Invalid SAP message.");
+ if (handle_cmd(conn, buf, bytes_read) < 0)
+ error("SAP protocol processing failure.");

return TRUE;
}
@@ -1151,7 +1137,7 @@ static void sap_connect_cb(GIOChannel *io, GError *gerr, gpointer data)
{
struct sap_connection *conn = data;

- DBG("io %p gerr %p data %p ", io, gerr, data);
+ DBG("conn %p, io %p", conn, io);

if (!conn)
return;
@@ -1170,13 +1156,13 @@ static void connect_auth_cb(DBusError *derr, void *data)
struct sap_connection *conn = data;
GError *gerr = NULL;

- DBG("derr %p data %p ", derr, data);
+ DBG("conn %p", conn);

if (!conn)
return;

if (derr && dbus_error_is_set(derr)) {
- error("Access denied: %s", derr->message);
+ error("Access has been denied (%s)", derr->message);
sap_conn_remove(conn);
return;
}
@@ -1188,7 +1174,7 @@ static void connect_auth_cb(DBusError *derr, void *data)
return;
}

- DBG("Client has been authorized.");
+ DBG("Access has been granted.");
}

static void connect_confirm_cb(GIOChannel *io, gpointer data)
@@ -1196,14 +1182,16 @@ static void connect_confirm_cb(GIOChannel *io, gpointer data)
struct sap_connection *conn = server->conn;
GError *gerr = NULL;
bdaddr_t src, dst;
+ char dstaddr[18];
int err;

- DBG("io %p data %p ", io, data);
+ DBG("conn %p io %p", conn, io);

if (!io)
return;

if (conn) {
+ DBG("Another SAP connection already exists.");
g_io_channel_shutdown(io, TRUE, NULL);
return;
}
@@ -1233,16 +1221,17 @@ static void connect_confirm_cb(GIOChannel *io, gpointer data)
return;
}

- err = btd_request_authorization(&src, &dst, SAP_UUID,
- connect_auth_cb, conn);
+ ba2str(&dst, dstaddr);
+
+ err = btd_request_authorization(&src, &dst, SAP_UUID, connect_auth_cb,
+ conn);
if (err < 0) {
- DBG("Authorization denied: %d %s", err, strerror(err));
+ error("Authorization failure (err %d)", err);
sap_conn_remove(conn);
return;
}

- DBG("SAP incoming connection (sock %d) authorization.",
- g_io_channel_unix_get_fd(io));
+ DBG("Authorizing incomming SAP connection from %s", dstaddr);
}

static inline DBusMessage *message_failed(DBusMessage *msg,
@@ -1257,8 +1246,6 @@ static DBusMessage *disconnect(DBusConnection *conn, DBusMessage *msg,
{
struct sap_server *server = data;

- DBG("server %p", server);
-
if (!server)
return message_failed(msg, "Server internal error.");

@@ -1332,8 +1319,8 @@ static void destroy_sap_interface(void *data)
{
struct sap_server *server = data;

- DBG("Unregistered interface %s on path %s",
- SAP_SERVER_INTERFACE, server->path);
+ DBG("Unregistered interface %s on path %s", SAP_SERVER_INTERFACE,
+ server->path);

server_free(server);
}
@@ -1394,7 +1381,7 @@ int sap_server_register(const char *path, bdaddr_t *src)
server_methods, server_signals, NULL,
server, destroy_sap_interface)) {
error("D-Bus failed to register %s interface",
- SAP_SERVER_INTERFACE);
+ SAP_SERVER_INTERFACE);
goto server_err;
}

--
1.7.4.1



2011-05-31 10:46:50

by Rymarkiewicz Waldemar

[permalink] [raw]
Subject: [PATCH v3 4/4] Simplify return value in disconnect_req

---
sap/server.c | 21 +++++++--------------
1 files changed, 7 insertions(+), 14 deletions(-)

diff --git a/sap/server.c b/sap/server.c
index c268cec..ec17759 100644
--- a/sap/server.c
+++ b/sap/server.c
@@ -349,11 +349,9 @@ static int disconnect_req(struct sap_connection *conn, uint8_t disc_type)

switch (disc_type) {
case SAP_DISCONNECTION_TYPE_GRACEFUL:
- if (conn->state == SAP_STATE_DISCONNECTED)
- goto error_req;
-
- if (conn->state == SAP_STATE_CONNECT_IN_PROGRESS)
- goto error_req;
+ if (conn->state == SAP_STATE_DISCONNECTED ||
+ conn->state == SAP_STATE_CONNECT_IN_PROGRESS)
+ return -EPERM;

if (conn->state == SAP_STATE_CONNECTED) {
conn->state = SAP_STATE_GRACEFUL_DISCONNECT;
@@ -367,11 +365,9 @@ static int disconnect_req(struct sap_connection *conn, uint8_t disc_type)
return 0;

case SAP_DISCONNECTION_TYPE_IMMEDIATE:
- if (conn->state == SAP_STATE_DISCONNECTED)
- goto error_req;
-
- if (conn->state == SAP_STATE_CONNECT_IN_PROGRESS)
- goto error_req;
+ if (conn->state == SAP_STATE_DISCONNECTED ||
+ conn->state == SAP_STATE_CONNECT_IN_PROGRESS)
+ return -EPERM;

if (conn->state == SAP_STATE_CONNECTED ||
conn->state == SAP_STATE_GRACEFUL_DISCONNECT) {
@@ -389,7 +385,7 @@ static int disconnect_req(struct sap_connection *conn, uint8_t disc_type)
if (conn->state != SAP_STATE_CONNECTED &&
conn->state != SAP_STATE_GRACEFUL_DISCONNECT) {
sap_error_rsp(conn);
- goto error_req;
+ return -EPERM;
}

conn->state = SAP_STATE_CLIENT_DISCONNECT;
@@ -404,9 +400,6 @@ static int disconnect_req(struct sap_connection *conn, uint8_t disc_type)
error("Unknown disconnection type (0x%02x).", disc_type);
return -EINVAL;
}
-
-error_req:
- return -EPERM;
}

static void transfer_apdu_req(struct sap_connection *conn,
--
1.7.4.1


2011-05-31 10:46:49

by Rymarkiewicz Waldemar

[permalink] [raw]
Subject: [PATCH v3 3/4] Coding style cleanup in sap server

---
sap/server.c | 23 +++++++++++------------
1 files changed, 11 insertions(+), 12 deletions(-)

diff --git a/sap/server.c b/sap/server.c
index aa8ba23..c268cec 100644
--- a/sap/server.c
+++ b/sap/server.c
@@ -257,7 +257,7 @@ static int send_message(struct sap_connection *conn, void *buf, size_t size)
DBG("conn %p, size %zu", conn, size);

gstatus = g_io_channel_write_chars(conn->io, buf, size, &written,
- &gerr);
+ &gerr);
if (gstatus != G_IO_STATUS_NORMAL) {
if (gerr)
g_error_free(gerr);
@@ -1095,7 +1095,7 @@ static gboolean sap_io_cb(GIOChannel *io, GIOCondition cond, gpointer data)
}

gstatus = g_io_channel_read_chars(io, buf, sizeof(buf) - 1,
- &bytes_read, &gerr);
+ &bytes_read, &gerr);
if (gstatus != G_IO_STATUS_NORMAL) {
if (gerr)
g_error_free(gerr);
@@ -1212,10 +1212,9 @@ static void connect_confirm_cb(GIOChannel *io, gpointer data)
conn->io = g_io_channel_ref(io);
conn->state = SAP_STATE_DISCONNECTED;

- bt_io_get(io, BT_IO_RFCOMM, &gerr,
- BT_IO_OPT_SOURCE_BDADDR, &src,
- BT_IO_OPT_DEST_BDADDR, &dst,
- BT_IO_OPT_INVALID);
+ bt_io_get(io, BT_IO_RFCOMM, &gerr, BT_IO_OPT_SOURCE_BDADDR, &src,
+ BT_IO_OPT_DEST_BDADDR, &dst, BT_IO_OPT_INVALID);
+
if (gerr) {
error("%s", gerr->message);
g_error_free(gerr);
@@ -1239,8 +1238,8 @@ static void connect_confirm_cb(GIOChannel *io, gpointer data)
static inline DBusMessage *message_failed(DBusMessage *msg,
const char *description)
{
- return g_dbus_create_error(msg, ERROR_INTERFACE ".Failed",
- "%s", description);
+ return g_dbus_create_error(msg, ERROR_INTERFACE ".Failed", "%s",
+ description);
}

static DBusMessage *disconnect(DBusConnection *conn, DBusMessage *msg,
@@ -1258,7 +1257,7 @@ static DBusMessage *disconnect(DBusConnection *conn, DBusMessage *msg,

if (disconnect_req(server->conn, SAP_DISCONNECTION_TYPE_GRACEFUL) < 0)
return g_dbus_create_error(msg, ERROR_INTERFACE ".Failed",
- "There is no active connection");
+ "There is no active connection");

return dbus_message_new_method_return(msg);
}
@@ -1287,7 +1286,7 @@ static DBusMessage *get_properties(DBusConnection *c,
DBUS_DICT_ENTRY_END_CHAR_AS_STRING, &dict);

connected = (conn->state == SAP_STATE_CONNECTED ||
- conn->state == SAP_STATE_GRACEFUL_DISCONNECT);
+ conn->state == SAP_STATE_GRACEFUL_DISCONNECT);
dict_append_entry(&dict, "Connected", DBUS_TYPE_BOOLEAN, &connected);

dbus_message_iter_close_container(&iter, &dict);
@@ -1380,8 +1379,8 @@ int sap_server_register(const char *path, bdaddr_t *src)
server->conn = NULL;

if (!g_dbus_register_interface(connection, path, SAP_SERVER_INTERFACE,
- server_methods, server_signals, NULL,
- server, destroy_sap_interface)) {
+ server_methods, server_signals, NULL, server,
+ destroy_sap_interface)) {
error("D-Bus failed to register %s interface",
SAP_SERVER_INTERFACE);
goto server_err;
--
1.7.4.1


2011-05-31 10:46:48

by Rymarkiewicz Waldemar

[permalink] [raw]
Subject: [PATCH v3 2/4] Fix return value in send_message

send_message returns the number of sent bytes and -EIO in case of
IO error now
---
sap/server.c | 12 +++++++-----
1 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/sap/server.c b/sap/server.c
index c5ea97c..aa8ba23 100644
--- a/sap/server.c
+++ b/sap/server.c
@@ -254,7 +254,7 @@ static int send_message(struct sap_connection *conn, void *buf, size_t size)
if (!conn || !buf)
return -EINVAL;

- DBG("size %zu", size);
+ DBG("conn %p, size %zu", conn, size);

gstatus = g_io_channel_write_chars(conn->io, buf, size, &written,
&gerr);
@@ -263,13 +263,15 @@ static int send_message(struct sap_connection *conn, void *buf, size_t size)
g_error_free(gerr);

error("write error (0x%02x).", gstatus);
- return -EINVAL;
+ return -EIO;
}

- if (written != size)
- error("write error.(written %zu size %zu)", written, size);
+ if (written != size) {
+ error("written %zu bytes out of %zu", written, size);
+ return -EIO;
+ }

- return 0;
+ return written;
}

static int disconnect_ind(void *sap_device, uint8_t disc_type)
--
1.7.4.1


2011-06-01 11:52:08

by Johan Hedberg

[permalink] [raw]
Subject: Re: [PATCH v3 3/4] Coding style cleanup in sap server

Hi Waldek,

On Tue, May 31, 2011, Waldemar Rymarkiewicz wrote:
> ---
> sap/server.c | 23 +++++++++++------------
> 1 files changed, 11 insertions(+), 12 deletions(-)

All four patches have been pushed upstream, but before that I fixed the
following issues with this patch:

> - bt_io_get(io, BT_IO_RFCOMM, &gerr,
> - BT_IO_OPT_SOURCE_BDADDR, &src,
> - BT_IO_OPT_DEST_BDADDR, &dst,
> - BT_IO_OPT_INVALID);
> + bt_io_get(io, BT_IO_RFCOMM, &gerr, BT_IO_OPT_SOURCE_BDADDR, &src,
> + BT_IO_OPT_DEST_BDADDR, &dst, BT_IO_OPT_INVALID);
> +

Readability trumps strict coding style adherence here, so it's fine if
each op_opt is on its own line.

> if (!g_dbus_register_interface(connection, path, SAP_SERVER_INTERFACE,
> - server_methods, server_signals, NULL,
> - server, destroy_sap_interface)) {
> + server_methods, server_signals, NULL, server,
> + destroy_sap_interface)) {

The idea is to try to have each continuation line past the opening ( of
the g_dbus_register_interface function call if possible while keeping
the lines under 80 charcters, so the existing split is fine except that
it can be indented by one more tab.

Johan