From: Ilia Kolomisnky <[email protected]>
According to BNEP spec. section 2.6.3 and
in order to pass TP/BNEP/CTRL/BV-02-C certification test.
Signed-off-by: Ilia Kolomisnky <[email protected]>
---
network/connection.c | 53 ++++++++++++++++++++++++++++++++++++++++---------
1 files changed, 43 insertions(+), 10 deletions(-)
diff --git a/network/connection.c b/network/connection.c
index ca1f4b2..5c3819a 100644
--- a/network/connection.c
+++ b/network/connection.c
@@ -49,6 +49,8 @@
#include "connection.h"
#define NETWORK_PEER_INTERFACE "org.bluez.Network"
+#define CON_SETUP_RETRIES 3
+#define CON_SETUP_TO_MS 9000
typedef enum {
CONNECTED,
@@ -73,6 +75,8 @@ struct network_conn {
guint watch; /* Disconnect watch */
guint dc_id;
struct network_peer *peer;
+ char con_attempt;
+ guint con_to_src;
};
struct __service_16 {
@@ -218,6 +222,8 @@ static gboolean bnep_setup_cb(GIOChannel *chan, GIOCondition cond,
goto failed;
}
+ g_source_remove(nc->con_to_src);
+
errno = EPROTO;
if ((size_t) r < sizeof(*rsp)) {
@@ -289,13 +295,11 @@ failed:
return FALSE;
}
-static int bnep_connect(struct network_conn *nc)
-{
+static inline int bnep_send_conn_req(struct network_conn *nc) {
+
struct bnep_setup_conn_req *req;
struct __service_16 *s;
- struct timeval timeo;
unsigned char pkt[BNEP_MTU];
- int fd;
/* Send request */
req = (void *) pkt;
@@ -306,14 +310,43 @@ static int bnep_connect(struct network_conn *nc)
s->dst = htons(nc->id);
s->src = htons(BNEP_SVC_PANU);
- memset(&timeo, 0, sizeof(timeo));
- timeo.tv_sec = 30;
+ if (send(g_io_channel_unix_get_fd(nc->io), pkt, sizeof(*req) + sizeof(*s), 0) < 0) {
+ error("bnep connection setup send failed");
+ return -errno;
+ }
- fd = g_io_channel_unix_get_fd(nc->io);
- setsockopt(fd, SOL_SOCKET, SO_RCVTIMEO, &timeo, sizeof(timeo));
+ nc->con_attempt++;
- if (send(fd, pkt, sizeof(*req) + sizeof(*s), 0) < 0)
- return -errno;
+ return 0;
+}
+
+static gboolean bnep_conn_req_to(gpointer user_data)
+{
+ struct network_conn *nc;
+ int err;
+
+ nc = (struct network_conn *) user_data;
+ if (nc->con_attempt == CON_SETUP_RETRIES) {
+ error("Too many bnep connection attempts, aborting connection setup");
+ }
+ else {
+ error("bnep connection setup TO, retrying...");
+
+ if (!bnep_send_conn_req(nc))
+ return TRUE;
+ }
+
+ cancel_connection(nc, "bnep setup failed");
+ return FALSE;
+}
+
+static int bnep_connect(struct network_conn *nc)
+{
+ int err;
+
+ if (err = bnep_send_conn_req(nc))
+ return err;
+ nc->con_to_src = g_timeout_add(CON_SETUP_TO_MS, bnep_conn_req_to, nc);
g_io_add_watch(nc->io, G_IO_IN | G_IO_ERR | G_IO_HUP | G_IO_NVAL,
(GIOFunc) bnep_setup_cb, nc);
--
1.7.4.1
Ping.
Regards
Ilia Kolominsky
[email protected]
Direct:=A0 +972(9)7906231
Mobile: +972(54)909009
> -----Original Message-----
> From: [email protected] [mailto:[email protected]]
> Sent: Sunday, January 22, 2012 9:11 AM
> To: [email protected]
> Cc: Ilia, Kolominsky
> Subject: [PATCH] BlueZ: Added retries for BNEP connection setup.
>=20
> From: Ilia Kolomisnky <[email protected]>
>=20
> According to BNEP spec. section 2.6.3 and
> in order to pass TP/BNEP/CTRL/BV-02-C certification test.
>=20
> Signed-off-by: Ilia Kolomisnky <[email protected]>
> ---
> network/connection.c | 53 ++++++++++++++++++++++++++++++++++++++++--
> -------
> 1 files changed, 43 insertions(+), 10 deletions(-)
>=20
> diff --git a/network/connection.c b/network/connection.c
> index ca1f4b2..5c3819a 100644
> --- a/network/connection.c
> +++ b/network/connection.c
> @@ -49,6 +49,8 @@
> #include "connection.h"
>=20
> #define NETWORK_PEER_INTERFACE "org.bluez.Network"
> +#define CON_SETUP_RETRIES 3
> +#define CON_SETUP_TO_MS 9000
>=20
> typedef enum {
> CONNECTED,
> @@ -73,6 +75,8 @@ struct network_conn {
> guint watch; /* Disconnect watch */
> guint dc_id;
> struct network_peer *peer;
> + char con_attempt;
> + guint con_to_src;
> };
>=20
> struct __service_16 {
> @@ -218,6 +222,8 @@ static gboolean bnep_setup_cb(GIOChannel *chan,
> GIOCondition cond,
> goto failed;
> }
>=20
> + g_source_remove(nc->con_to_src);
> +
> errno =3D EPROTO;
>=20
> if ((size_t) r < sizeof(*rsp)) {
> @@ -289,13 +295,11 @@ failed:
> return FALSE;
> }
>=20
> -static int bnep_connect(struct network_conn *nc)
> -{
> +static inline int bnep_send_conn_req(struct network_conn *nc) {
> +
> struct bnep_setup_conn_req *req;
> struct __service_16 *s;
> - struct timeval timeo;
> unsigned char pkt[BNEP_MTU];
> - int fd;
>=20
> /* Send request */
> req =3D (void *) pkt;
> @@ -306,14 +310,43 @@ static int bnep_connect(struct network_conn *nc)
> s->dst =3D htons(nc->id);
> s->src =3D htons(BNEP_SVC_PANU);
>=20
> - memset(&timeo, 0, sizeof(timeo));
> - timeo.tv_sec =3D 30;
> + if (send(g_io_channel_unix_get_fd(nc->io), pkt, sizeof(*req) +
> sizeof(*s), 0) < 0) {
> + error("bnep connection setup send failed");
> + return -errno;
> + }
>=20
> - fd =3D g_io_channel_unix_get_fd(nc->io);
> - setsockopt(fd, SOL_SOCKET, SO_RCVTIMEO, &timeo, sizeof(timeo));
> + nc->con_attempt++;
>=20
> - if (send(fd, pkt, sizeof(*req) + sizeof(*s), 0) < 0)
> - return -errno;
> + return 0;
> +}
> +
> +static gboolean bnep_conn_req_to(gpointer user_data)
> +{
> + struct network_conn *nc;
> + int err;
> +
> + nc =3D (struct network_conn *) user_data;
> + if (nc->con_attempt =3D=3D CON_SETUP_RETRIES) {
> + error("Too many bnep connection attempts, aborting
> connection setup");
> + }
> + else {
> + error("bnep connection setup TO, retrying...");
> +
> + if (!bnep_send_conn_req(nc))
> + return TRUE;
> + }
> +
> + cancel_connection(nc, "bnep setup failed");
> + return FALSE;
> +}
> +
> +static int bnep_connect(struct network_conn *nc)
> +{
> + int err;
> +
> + if (err =3D bnep_send_conn_req(nc))
> + return err;
> + nc->con_to_src =3D g_timeout_add(CON_SETUP_TO_MS, bnep_conn_req_to,
> nc);
>=20
> g_io_add_watch(nc->io, G_IO_IN | G_IO_ERR | G_IO_HUP | G_IO_NVAL,
> (GIOFunc) bnep_setup_cb, nc);
> --
> 1.7.4.1
Hi Johan,
Thanks for the review, I will send patch v2 shortly.
> -----Original Message-----
> From: Johan Hedberg [mailto:[email protected]]
> Sent: Thursday, February 02, 2012 8:56 PM
> To: [email protected]
> Cc: [email protected]; Ilia, Kolominsky
> Subject: Re: [PATCH] BlueZ: Added retries for BNEP connection setup.
>
> Hi Ilia,
>
> On Sun, Jan 22, 2012, [email protected] wrote:
> > According to BNEP spec. section 2.6.3 and
> > in order to pass TP/BNEP/CTRL/BV-02-C certification test.
>
> Please add a much more detailed commit message explaining the
> background
> to this, what the test case does and why your fix is the right one, why
> using SO_RCVTIMEO wasn't good enough, etc. Also fix the summary line
> not
> to have a "BlueZ" prefix (that should be within the initial []) and the
> form of the first word should be "Add" and not "Added" (actually since
> this entire patch seems to be a fix it should be "Fix ...").
>
> > Signed-off-by: Ilia Kolomisnky <[email protected]>
>
> We don't use this for user space patches.
>
> > +#define CON_SETUP_TO_MS 9000
>
> Please use the _seconds variant of the GLib timeout functions.
>
> > typedef enum {
> > CONNECTED,
> > @@ -73,6 +75,8 @@ struct network_conn {
> > guint watch; /* Disconnect watch */
> > guint dc_id;
> > struct network_peer *peer;
> > + char con_attempt;
>
> Don't use char for integer variables. int, guint etc. would be more
> appropriate here.
>
> > + guint con_to_src;
>
> I'd call this timeout_source (it's already within a struct called
> network_conn so you don't need a conn prefix).
>
> > + g_source_remove(nc->con_to_src);
>
> I suppose this should also set the source back to 0?
>
> > +static inline int bnep_send_conn_req(struct network_conn *nc) {
>
> Does this really need to be inline? Have you done some profiling to
> determine that it's really needed and has a huge impact? If not just
> leave the "inline" bit out.
>
> > - int fd;
>
> Please keep this variable.
>
> > @@ -306,14 +310,43 @@ static int bnep_connect(struct network_conn
> *nc)
> > s->dst = htons(nc->id);
> > s->src = htons(BNEP_SVC_PANU);
> >
> > - memset(&timeo, 0, sizeof(timeo));
> > - timeo.tv_sec = 30;
> > + if (send(g_io_channel_unix_get_fd(nc->io), pkt, sizeof(*req) +
> sizeof(*s), 0) < 0) {
>
> ..and don't do the get_fd() call within the parameters of another
> function call. Also, why not use write() since you're anyway passing 0
> as the flags (I know the original code was using send so this is a
> minor
> issue). Also the above line looks like it's breaking the 80 character
> rule.
>
> > +static gboolean bnep_conn_req_to(gpointer user_data)
> > +{
> > + struct network_conn *nc;
> > + int err;
> > +
> > + nc = (struct network_conn *) user_data;
>
> No explicit type casts for void pointer please.
>
> > + if (nc->con_attempt == CON_SETUP_RETRIES) {
> > + error("Too many bnep connection attempts, aborting
> connection setup");
> > + }
> > + else {
> > + error("bnep connection setup TO, retrying...");
> > +
> > + if (!bnep_send_conn_req(nc))
> > + return TRUE;
> > + }
>
> Looks like you're breaking the 80 character rule for the first error
> call. Also, the coding style should be "} else {" on one line.
>
> > +static int bnep_connect(struct network_conn *nc)
> > +{
> > + int err;
> > +
> > + if (err = bnep_send_conn_req(nc))
> > + return err;
>
> Split this up:
>
> err = foo();
> if (err < 0)
> return err;
>
> > + nc->con_to_src = g_timeout_add(CON_SETUP_TO_MS, bnep_conn_req_to,
> nc);
>
> And this one (like I mentioned previously) should use the _seconds
> variant.
>
> Johan
Ilia
Hi Ilia,
On Sun, Jan 22, 2012, [email protected] wrote:
> According to BNEP spec. section 2.6.3 and
> in order to pass TP/BNEP/CTRL/BV-02-C certification test.
Please add a much more detailed commit message explaining the background
to this, what the test case does and why your fix is the right one, why
using SO_RCVTIMEO wasn't good enough, etc. Also fix the summary line not
to have a "BlueZ" prefix (that should be within the initial []) and the
form of the first word should be "Add" and not "Added" (actually since
this entire patch seems to be a fix it should be "Fix ...").
> Signed-off-by: Ilia Kolomisnky <[email protected]>
We don't use this for user space patches.
> +#define CON_SETUP_TO_MS 9000
Please use the _seconds variant of the GLib timeout functions.
> typedef enum {
> CONNECTED,
> @@ -73,6 +75,8 @@ struct network_conn {
> guint watch; /* Disconnect watch */
> guint dc_id;
> struct network_peer *peer;
> + char con_attempt;
Don't use char for integer variables. int, guint etc. would be more
appropriate here.
> + guint con_to_src;
I'd call this timeout_source (it's already within a struct called
network_conn so you don't need a conn prefix).
> + g_source_remove(nc->con_to_src);
I suppose this should also set the source back to 0?
> +static inline int bnep_send_conn_req(struct network_conn *nc) {
Does this really need to be inline? Have you done some profiling to
determine that it's really needed and has a huge impact? If not just
leave the "inline" bit out.
> - int fd;
Please keep this variable.
> @@ -306,14 +310,43 @@ static int bnep_connect(struct network_conn *nc)
> s->dst = htons(nc->id);
> s->src = htons(BNEP_SVC_PANU);
>
> - memset(&timeo, 0, sizeof(timeo));
> - timeo.tv_sec = 30;
> + if (send(g_io_channel_unix_get_fd(nc->io), pkt, sizeof(*req) + sizeof(*s), 0) < 0) {
..and don't do the get_fd() call within the parameters of another
function call. Also, why not use write() since you're anyway passing 0
as the flags (I know the original code was using send so this is a minor
issue). Also the above line looks like it's breaking the 80 character
rule.
> +static gboolean bnep_conn_req_to(gpointer user_data)
> +{
> + struct network_conn *nc;
> + int err;
> +
> + nc = (struct network_conn *) user_data;
No explicit type casts for void pointer please.
> + if (nc->con_attempt == CON_SETUP_RETRIES) {
> + error("Too many bnep connection attempts, aborting connection setup");
> + }
> + else {
> + error("bnep connection setup TO, retrying...");
> +
> + if (!bnep_send_conn_req(nc))
> + return TRUE;
> + }
Looks like you're breaking the 80 character rule for the first error
call. Also, the coding style should be "} else {" on one line.
> +static int bnep_connect(struct network_conn *nc)
> +{
> + int err;
> +
> + if (err = bnep_send_conn_req(nc))
> + return err;
Split this up:
err = foo();
if (err < 0)
return err;
> + nc->con_to_src = g_timeout_add(CON_SETUP_TO_MS, bnep_conn_req_to, nc);
And this one (like I mentioned previously) should use the _seconds
variant.
Johan