Patches

Hey,
Was scrolling across some of your code and saw some syntactical touching up
and optimizations that could be made. They're attached, if you'd like to
commit them.
Best,
Matthew Daiter

diff --git a/openbsc/src/osmo-bsc/osmo_bsc_filter.c b/openbsc/src/osmo-bsc/osmo_bsc_filter.c
index 5ad9fd7..6b15886 100644
--- a/openbsc/src/osmo-bsc/osmo_bsc_filter.c
+++ b/openbsc/src/osmo-bsc/osmo_bsc_filter.c
@@ -47,7 +47,7 @@ static void handle_lu_request(struct gsm_subscriber_connection *conn,
 	gsm48_generate_lai(&lai, net->country_code, net->network_code,
 			   conn->bts->location_area_code);
 
-	if (memcmp(&lai, &lu->lai, sizeof(lai)) != 0) {
+	if (memcmp(&lai, &lu->lai, sizeof(lai))) {
 		LOGP(DMSC, LOGL_DEBUG, "Marking con for welcome USSD.\n");
 		conn->sccp_con->new_subscriber = 1;
 	}
@@ -314,13 +314,10 @@ static int bsc_patch_mm_info(struct gsm_subscriber_connection *conn,
 
 static int has_core_identity(struct osmo_msc_data *msc)
 {
-	if (msc->core_mnc != -1)
-		return 1;
-	if (msc->core_mcc != -1)
-		return 1;
-	if (msc->core_lac != -1)
-		return 1;
-	if (msc->core_ci != -1)
+	if ((msc->core_mnc != -1) ||
+	    (msc->core_mcc != -1) ||
+	    (msc->core_lac != -1) ||
+	    (msc->core_ci != -1))
 		return 1;
 	return 0;
 }
@@ -367,7 +364,6 @@ int bsc_scan_msc_msg(struct gsm_subscriber_connection *conn, struct msgb *msg)
 
 		if (conn->sccp_con->new_subscriber)
 			return send_welcome_ussd(conn);
-		return 0;
 	} else if (mtype == GSM48_MT_MM_INFO) {
 		bsc_patch_mm_info(conn, &gh->data[0], length);
 	}

Hi Matthew,

thanks for your patches and your interest in contributing.

On Tue, Apr 05, 2016 at 11:18:51PM +0200, Matthew Daiter wrote:
> -		if (!msc->msc_con->is_authenticated)
> -			continue;
> -		if (!is_emerg && msc->type != MSC_CON_TYPE_NORMAL)
> -			continue;
> -		if (is_emerg && !msc->allow_emerg)
> +		if ((!msc->msc_con->is_authenticated) ||
> +		    (!is_emerg && msc->type != MSC_CON_TYPE_NORMAL) ||
> +		    (is_emerg && !msc->allow_emerg))
>  			continue;

I thinkt it's a matter of taste.  To me, the existing code actually is
more obvious and easier to read, as convolutd nested parenthesis in a
single 'if' statement, where there are more possible relationship
of the individual conditions.

> -	if (memcmp(&lai, &lu->lai, sizeof(lai)) != 0) {
> +	if (memcmp(&lai, &lu->lai, sizeof(lai))) {

again here, it is a question of taste.  The '!= 0' proabably serves as
a reminder at the strange behavior of memcmp() returning 0 in case of
success.

So I'm sorry, but I wouldn't merge any of your patches, I don't think
they make the core more readable or improve it in any other way :(

I'd hope you could focus your interest in contributing into a different
area that actually makes a difference to the OpenBSC user community.
Thanks!

On Wed, Apr 06, 2016 at 10:05:46AM +0200, Harald Welte wrote:
> Hi Matthew,
> 
> thanks for your patches and your interest in contributing.
> 
> On Tue, Apr 05, 2016 at 11:18:51PM +0200, Matthew Daiter wrote:
> > -		if (!msc->msc_con->is_authenticated)
> > -			continue;
> > -		if (!is_emerg && msc->type != MSC_CON_TYPE_NORMAL)
> > -			continue;
> > -		if (is_emerg && !msc->allow_emerg)
> > +		if ((!msc->msc_con->is_authenticated) ||
> > +		    (!is_emerg && msc->type != MSC_CON_TYPE_NORMAL) ||
> > +		    (is_emerg && !msc->allow_emerg))
> >  			continue;
> 
> I thinkt it's a matter of taste.  To me, the existing code actually is
> more obvious and easier to read

I also prefer the separate if()s for instant clarity.

> > -	if (memcmp(&lai, &lu->lai, sizeof(lai)) != 0) {
> > +	if (memcmp(&lai, &lu->lai, sizeof(lai))) {
> 
> again here, it is a question of taste.  The '!= 0' proabably serves as
> a reminder at the strange behavior of memcmp() returning 0 in case of
> success.

I always do the != 0 with strcmp :)
Though I wouldn't bother to commit a patch to change it, I agree with != 0.

Thanks for joining us, Matthew! Do stay around and help us out :)

~Neels