Hi Johan, Thanks for the review, I will send patch v2 shortly. > -----Original Message----- > From: Johan Hedberg [mailto:johan.hedberg@xxxxxxxxx] > Sent: Thursday, February 02, 2012 8:56 PM > To: ilia.kolominsky@xxxxxxxxx > Cc: linux-bluetooth@xxxxxxxxxxxxxxx; Ilia, Kolominsky > Subject: Re: [PATCH] BlueZ: Added retries for BNEP connection setup. > > Hi Ilia, > > On Sun, Jan 22, 2012, ilia.kolominsky@xxxxxxxxx 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 <iliak@xxxxxx> > > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
