[PATCH 1/3] Remove stray newlines in DBG and ofono log messages

[PATCH 1/3] Remove stray newlines in DBG and ofono log messages

[=?unknown-8bit?q?R=C3=A9mi?= Denis-Courmont]

[-- Attachment #1: Type: text/plain, Size: 8835 bytes --]

---
 drivers/isimodem/call-barring.c    |   10 +++++-----
 drivers/isimodem/call-forwarding.c |    8 ++++----
 drivers/isimodem/call-settings.c   |    8 ++++----
 drivers/isimodem/cbs.c             |    2 +-
 drivers/isimodem/sms.c             |    2 +-
 drivers/stemodem/caif_rtnl.c       |    2 +-
 drivers/stemodem/gprs-context.c    |    2 +-
 plugins/isigen.c                   |    2 +-
 src/gprs.c                         |    2 +-
 src/sim.c                          |    2 +-
 src/voicecall.c                    |    4 ++--
 11 files changed, 22 insertions(+), 22 deletions(-)

diff --git a/drivers/isimodem/call-barring.c b/drivers/isimodem/call-barring.c
index 2a982f9..7cdd218 100644
--- a/drivers/isimodem/call-barring.c
+++ b/drivers/isimodem/call-barring.c
@@ -125,7 +125,7 @@ static void isi_set(struct ofono_call_barring *barr, const char *lock,
 		0, 0			/* Filler */
 	};
 
-	DBG("lock code %s enable %d class %d password %s\n",
+	DBG("lock code %s enable %d class %d password %s",
 		lock, enable, cls, passwd);
 
 	if (!cbd || !bd)
@@ -177,7 +177,7 @@ static void update_status_mask(unsigned int *mask, int bsc)
 		break;
 
 	default:
-		DBG("Unknown BSC: 0x%04X\n", bsc);
+		DBG("Unknown BSC: 0x%04X", bsc);
 		break;
 	}
 }
@@ -244,7 +244,7 @@ static gboolean query_resp_cb(GIsiClient *client,
 		}
 	}
 
-	DBG("mask=0x%04X\n", mask);
+	DBG("mask=0x%04X", mask);
 	CALLBACK_WITH_SUCCESS(cb, mask, cbd->data);
 	goto out;
 
@@ -273,7 +273,7 @@ static void isi_query(struct ofono_call_barring *barr, const char *lock,
 		0				/* Subblock count */
 	};
 
-	DBG("barring query lock code %s\n", lock);
+	DBG("barring query lock code %s", lock);
 
 	if (!cbd || !bd)
 		goto error;
@@ -343,7 +343,7 @@ static void isi_set_passwd(struct ofono_call_barring *barr, const char *lock,
 		0, 0				/* Filler */
 	};
 
-	DBG("lock code %s (%u) old password %s new password %s\n",
+	DBG("lock code %s (%u) old password %s new password %s",
 		lock, ss_code, old_passwd, new_passwd);
 
 	if (!cbd || !bd)
diff --git a/drivers/isimodem/call-forwarding.c b/drivers/isimodem/call-forwarding.c
index 1448451..bb25ee7 100644
--- a/drivers/isimodem/call-forwarding.c
+++ b/drivers/isimodem/call-forwarding.c
@@ -70,7 +70,7 @@ static int forw_type_to_isi_code(int type)
 		ss_code = SS_GSM_ALL_COND_FORWARDINGS;
 		break;
 	default:
-		DBG("Unknown forwarding type %d\n", type);
+		DBG("Unknown forwarding type %d", type);
 		ss_code = -1;
 		break;
 	}
@@ -227,7 +227,7 @@ static void isi_registration(struct ofono_call_forwarding *cf,
 	/* Followed by number in UCS-2, zero sub address bytes, and 0
 	 * to 3 bytes of filler */
 
-	DBG("forwarding type %d class %d\n", type, cls);
+	DBG("forwarding type %d class %d", type, cls);
 
 	if (!cbd || !fd || !number->number || strlen(number->number) > 28)
 		goto error;
@@ -347,7 +347,7 @@ static void isi_erasure(struct ofono_call_forwarding *cf, int type, int cls,
 		0  /* Subblock count */
 	};
 
-	DBG("forwarding type %d class %d\n", type, cls);
+	DBG("forwarding type %d class %d", type, cls);
 
 	if (!cbd || !fd)
 		goto error;
@@ -479,7 +479,7 @@ static void isi_query(struct ofono_call_forwarding *cf, int type, int cls,
 		0  /* Subblock count */
 	};
 
-	DBG("forwarding type %d class %d\n", type, cls);
+	DBG("forwarding type %d class %d", type, cls);
 
 	if (!cbd || !fd || cls != 7)
 		goto error;
diff --git a/drivers/isimodem/call-settings.c b/drivers/isimodem/call-settings.c
index c845d54..23c1982 100644
--- a/drivers/isimodem/call-settings.c
+++ b/drivers/isimodem/call-settings.c
@@ -84,7 +84,7 @@ static void update_status_mask(unsigned int *mask, int bsc)
 		break;
 
 	default:
-		DBG("Unknown BSC value %d, please report\n", bsc);
+		DBG("Unknown BSC value %d, please report", bsc);
 		break;
 	}
 }
@@ -146,7 +146,7 @@ static gboolean query_resp_cb(GIsiClient *client,
 		}
 	}
 
-	DBG("status_mask %d\n", mask);
+	DBG("status_mask %d", mask);
 	CALLBACK_WITH_SUCCESS(cb, mask, cbd->data);
 	goto out;
 
@@ -175,7 +175,7 @@ static void isi_cw_query(struct ofono_call_settings *cs, int cls,
 		0 /* Subblock count */
 	};
 
-	DBG("waiting class %d\n", cls);
+	DBG("waiting class %d", cls);
 
 	if (!cbd || !sd)
 		goto error;
@@ -266,7 +266,7 @@ static void isi_cw_set(struct ofono_call_settings *cs, int mode, int cls,
 		0  /* Subblock count */
 	};
 
-	DBG("waiting mode %d class %d\n", mode, cls);
+	DBG("waiting mode %d class %d", mode, cls);
 
 	if (!cbd || !sd)
 		goto error;
diff --git a/drivers/isimodem/cbs.c b/drivers/isimodem/cbs.c
index dec8154..2a32720 100644
--- a/drivers/isimodem/cbs.c
+++ b/drivers/isimodem/cbs.c
@@ -104,7 +104,7 @@ static gboolean routing_resp_cb(GIsiClient *client,
 				"It appears some other component is "
 				"already\n  registered as the CBS "
 				"routing endpoint.\n  As a consequence, "
-				"receiving CBSs is NOT going to work.\n\n",
+				"receiving CBSs is NOT going to work.\n",
 				msg[1], sms_isi_cause_name(msg[1]));
 		return TRUE;
 	}
diff --git a/drivers/isimodem/sms.c b/drivers/isimodem/sms.c
index 319eb54..e2c2533 100644
--- a/drivers/isimodem/sms.c
+++ b/drivers/isimodem/sms.c
@@ -524,7 +524,7 @@ static gboolean routing_resp_cb(GIsiClient *client,
 				"already\n  registered as the SMS "
 				"routing endpoint.\n  As a consequence, "
 				"receiving SMSs is NOT going to work.\n  "
-				"On the other hand, sending might work.\n\n",
+				"On the other hand, sending might work.\n",
 				msg[1], sms_isi_cause_name(msg[1]));
 			ofono_sms_register(sms);
 		}
diff --git a/drivers/stemodem/caif_rtnl.c b/drivers/stemodem/caif_rtnl.c
index 4c00446..c327d57 100644
--- a/drivers/stemodem/caif_rtnl.c
+++ b/drivers/stemodem/caif_rtnl.c
@@ -155,7 +155,7 @@ static int add_attribute(struct nlmsghdr *n, unsigned int maxlen, int type,
 	struct rtattr *rta;
 
 	if ((NLMSG_ALIGN(n->nlmsg_len) + RTA_ALIGN(len)) > maxlen) {
-		DBG("attribute to large for message %d %d %d\n",
+		DBG("attribute to large for message %d %d %d",
 				n->nlmsg_len, len, maxlen);
 		return -1;
 	}
diff --git a/drivers/stemodem/gprs-context.c b/drivers/stemodem/gprs-context.c
index 05fec3f..1675cd8 100644
--- a/drivers/stemodem/gprs-context.c
+++ b/drivers/stemodem/gprs-context.c
@@ -130,7 +130,7 @@ static void text_handler(GMarkupParseContext *context,
 static void error_handler(GMarkupParseContext *context,
 				GError *error, gpointer user_data)
 {
-	DBG("Error parsing xml response from eppsd: %s\n",
+	DBG("Error parsing xml response from eppsd: %s",
 		error->message);
 }
 
diff --git a/plugins/isigen.c b/plugins/isigen.c
index fad4e20..3384e9f 100644
--- a/plugins/isigen.c
+++ b/plugins/isigen.c
@@ -290,7 +290,7 @@ static int isigen_probe(struct ofono_modem *modem)
 	if (address) {
 		int error = g_pn_netlink_set_address(idx, address);
 		if (error && error != -EEXIST) {
-			DBG("g_pn_netlink_set_address: %s\n", strerror(-error));
+			DBG("g_pn_netlink_set_address: %s", strerror(-error));
 			g_pn_netlink_stop(link);
 			return -errno;
 		}
diff --git a/src/gprs.c b/src/gprs.c
index 3de3f2b..7e42a99 100644
--- a/src/gprs.c
+++ b/src/gprs.c
@@ -1730,7 +1730,7 @@ static DBusMessage *gprs_remove_context(DBusConnection *conn,
 		storage_sync(gprs->imsi, SETTINGS_STORE, gprs->settings);
 	}
 
-	DBG("Unregistering context: %s\n", ctx->path);
+	DBG("Unregistering context: %s", ctx->path);
 	context_dbus_unregister(ctx);
 	gprs->contexts = g_slist_remove(gprs->contexts, ctx);
 
diff --git a/src/sim.c b/src/sim.c
index c4af079..bc7859c 100644
--- a/src/sim.c
+++ b/src/sim.c
@@ -1097,7 +1097,7 @@ static void sim_sdn_read_cb(int ok, int length, int record,
 	if (sim->service_numbers &&
 			g_slist_find_custom(sim->service_numbers,
 				alpha, service_number_compare)) {
-		ofono_error("Duplicate EFsdn entries for `%s'\n",
+		ofono_error("Duplicate EFsdn entries for `%s'",
 				alpha);
 		g_free(alpha);
 
diff --git a/src/voicecall.c b/src/voicecall.c
index 045b492..874f958 100644
--- a/src/voicecall.c
+++ b/src/voicecall.c
@@ -1829,7 +1829,7 @@ void ofono_voicecall_notify(struct ofono_voicecall *vc,
 				call_compare_by_id);
 
 	if (l) {
-		DBG("Found call with id: %d\n", call->id);
+		DBG("Found call with id: %d", call->id);
 		voicecall_set_call_status(l->data, call->status);
 		voicecall_set_call_lineid(l->data, &call->phone_number,
 						call->clip_validity);
@@ -1837,7 +1837,7 @@ void ofono_voicecall_notify(struct ofono_voicecall *vc,
 		return;
 	}
 
-	DBG("Did not find a call with id: %d\n", call->id);
+	DBG("Did not find a call with id: %d", call->id);
 
 	__ofono_modem_callid_hold(modem, call->id);
 
-- 
1.7.1

[PATCH 2/3] Use %m instead of strerror() in syslog messages

[=?unknown-8bit?q?R=C3=A9mi?= Denis-Courmont]

[-- Attachment #1: Type: text/plain, Size: 6825 bytes --]

strerror() is not thread-safe, nor in the glibc implementation,
nor in the POSIX specification. In my experience, it will infrequently
crash when another thread uses locale data in any way, including
string formatting and such...

Since the Huawei driver uses thread, we might as well avoid strerror().
---
 drivers/isimodem/sim.c |    3 ++-
 plugins/hfp.c          |    4 ++--
 plugins/isigen.c       |   13 ++++++++-----
 plugins/n900.c         |   14 +++++++-------
 plugins/nokia-gpio.c   |   29 +++++++++++++++++------------
 5 files changed, 36 insertions(+), 27 deletions(-)

diff --git a/drivers/isimodem/sim.c b/drivers/isimodem/sim.c
index d03a07e..6379af8 100644
--- a/drivers/isimodem/sim.c
+++ b/drivers/isimodem/sim.c
@@ -428,7 +428,8 @@ static void sim_reachable_cb(GIsiClient *client, gboolean alive,
 	struct ofono_sim *sim = opaque;
 
 	if (!alive) {
-		DBG("SIM client: %s", strerror(-g_isi_client_error(client)));
+		errno = -g_isi_client_error(client);
+		DBG("SIM client: %m");
 		ofono_sim_remove(sim);
 		return;
 	}
diff --git a/plugins/hfp.c b/plugins/hfp.c
index 0a19dac..2169d40 100644
--- a/plugins/hfp.c
+++ b/plugins/hfp.c
@@ -324,8 +324,8 @@ static int service_level_connection(struct ofono_modem *modem, int fd)
 
 	io = g_io_channel_unix_new(fd);
 	if (!io) {
-		ofono_error("Service level connection failed: %s (%d)",
-			strerror(errno), errno);
+		ofono_error("Service level connection failed: %m (%d)",
+			errno);
 		return -EIO;
 	}
 
diff --git a/plugins/isigen.c b/plugins/isigen.c
index 3384e9f..5eb2a96 100644
--- a/plugins/isigen.c
+++ b/plugins/isigen.c
@@ -217,7 +217,8 @@ static void reachable_cb(GIsiClient *client, gboolean alive, uint16_t object,
 	};
 
 	if (!alive) {
-		DBG("MTC client: %s", strerror(-g_isi_client_error(client)));
+		errno = -g_isi_client_error(client);
+		DBG("MTC client: %m");
 
 		if (isi->linkstate == PN_LINK_UP)
 			g_isi_send(client, msg, sizeof(msg),
@@ -272,25 +273,27 @@ static int isigen_probe(struct ofono_modem *modem)
 
 	idx = g_isi_modem_by_name(ifname);
 	if (idx == NULL) {
-		DBG("Interface=%s: %s", ifname, strerror(errno));
+		DBG("Interface=%s: %m", ifname);
 		return -errno;
 	}
 
 	if (g_pn_netlink_by_modem(idx)) {
-		DBG("%s: %s", ifname, strerror(EBUSY));
+		errno = EBUSY;
+		DBG("%s: %m", ifname);
 		return -EBUSY;
 	}
 
 	link = g_pn_netlink_start(idx, phonet_status_cb, modem);
 	if (!link) {
-		DBG("%s: %s", ifname, strerror(errno));
+		DBG("%s: %m", ifname);
 		return -errno;
 	}
 
 	if (address) {
 		int error = g_pn_netlink_set_address(idx, address);
 		if (error && error != -EEXIST) {
-			DBG("g_pn_netlink_set_address: %s", strerror(-error));
+			errno = -error;
+			DBG("g_pn_netlink_set_address: %m");
 			g_pn_netlink_stop(link);
 			return -errno;
 		}
diff --git a/plugins/n900.c b/plugins/n900.c
index a9e6b59..4e27d20 100644
--- a/plugins/n900.c
+++ b/plugins/n900.c
@@ -218,8 +218,7 @@ static gboolean mtc_startup_synq_cb(GIsiClient *client,
 	const unsigned char *msg = data;
 
 	if (!msg) {
-		DBG("%s: %s", "MTC_STARTUP_SYNQ",
-			strerror(-g_isi_client_error(client)));
+		DBG("%s: %m", "MTC_STARTUP_SYNQ");
 		return TRUE;
 	}
 
@@ -280,7 +279,8 @@ static void mtc_reachable_cb(GIsiClient *client, gboolean alive,
 	struct isi_data *isi = opaque;
 
 	if (!alive) {
-		DBG("MTC client: %s", strerror(-g_isi_client_error(client)));
+		errno = -g_isi_client_error(client);
+		DBG("MTC client: %m");
 		/* enable is terminated eventually by timeout */
 		return;
 	}
@@ -332,8 +332,8 @@ static gboolean mtc_power_off_cb(GIsiClient *client,
 	const unsigned char *msg = data;
 
 	if (!msg) {
-		DBG("%s: %s", "MTC_POWER_OFF_RESP",
-			strerror(-g_isi_client_error(client)));
+		errno = -g_isi_client_error(client);
+		DBG("%s: %m", "MTC_POWER_OFF_RESP");
 
 		if (isi->power_state == POWER_STATE_OFF_STARTED)
 			mtc_power_off(isi);
@@ -399,12 +399,12 @@ static int n900_probe(struct ofono_modem *modem)
 
 	idx = g_isi_modem_by_name(ifname);
 	if (!idx) {
-		DBG("Interface=%s: %s", ifname, strerror(errno));
+		DBG("Interface=%s: %m", ifname);
 		return -errno;
 	}
 
 	if (gpio_probe(idx, address, n900_power_cb, modem) != 0) {
-		DBG("gpio for %s: %s", ifname, strerror(errno));
+		DBG("gpio for %s: %m", ifname);
 		return -errno;
 	}
 
diff --git a/plugins/nokia-gpio.c b/plugins/nokia-gpio.c
index f3b9460..1a08ff9 100644
--- a/plugins/nokia-gpio.c
+++ b/plugins/nokia-gpio.c
@@ -156,7 +156,7 @@ static int file_write(char const *filename, char const *output)
 
 	f = fopen(filename, "r+");
 	if (!f) {
-		DBG("%s: %s (%d)", filename, strerror(errno), errno);
+		DBG("%s: %m (%d)", filename, errno);
 		return -1;
 	}
 
@@ -647,14 +647,14 @@ static int gpio_probe_links(void)
 	if (!dir_exists(cmtdir)) {
 
 		if (mkdir(cmtdir, 0755) == -1) {
-			DBG("%s: %s", cmtdir, strerror(errno));
+			DBG("%s: %m", cmtdir);
 			return -(errno = ENODEV);
 		}
 	}
 
 	gpio = opendir(gpiodir);
 	if (gpio == NULL) {
-		DBG("%s: %s", "gpiodir", strerror(errno));
+		DBG("%s: %m", "gpiodir");
 		return -(errno = ENODEV);
 	}
 
@@ -670,14 +670,14 @@ static int gpio_probe_links(void)
 
 		nf = fopen(nn, "rb");
 		if (nf == NULL) {
-			DBG("%s: %s", nn, strerror(errno));
+			DBG("%s: %m", nn);
 			continue;
 		}
 
 		len = fread(name, sizeof name, 1, nf);
 
 		if (ferror(nf)) {
-			DBG("read from %s: %s", nn, strerror(errno));
+			DBG("read from %s: %m", nn);
 			fclose(nf);
 			continue;
 		}
@@ -696,10 +696,10 @@ static int gpio_probe_links(void)
 		snprintf(to, sizeof to, "%s/%s", cmtdir, name);
 
 		if (symlink(from, to) == -1)
-			DBG("%s: %s", to, strerror(errno));
+			DBG("%s: %m", to);
 	}
 
-	DBG("%s: %s", "/sys/class/gpio", strerror(errno));
+	DBG("%s: %m", "/sys/class/gpio");
 
 	return -(errno = ENODEV);
 }
@@ -715,13 +715,15 @@ int gpio_probe(GIsiModem *idx, unsigned addr, gpio_finished_cb_t cb, void *data)
 	}
 
 	if (self.callback) {
-		DBG("gpio: %s", strerror(EBUSY));
-		return -(errno = EBUSY);
+		errno = EBUSY;
+		DBG("gpio: %m");
+		return -EBUSY;
 	}
 
 	if (g_pn_netlink_by_modem(idx)) {
-		DBG("Phonet link %p: %s", idx, strerror(EBUSY));
-		return -(errno = EBUSY);
+		errno = EBUSY;
+		DBG("Phonet link %p: %m", idx);
+		return -EBUSY;
 	}
 
 	self.target = PHONET_LINK_NONE;
@@ -765,7 +767,10 @@ int gpio_probe(GIsiModem *idx, unsigned addr, gpio_finished_cb_t cb, void *data)
 	if (addr) {
 		error = g_pn_netlink_set_address(idx, addr);
 		if (error && error != -EEXIST)
-			DBG("g_pn_netlink_set_address: %s", strerror(-error));
+		{
+			errno = -error;
+			DBG("g_pn_netlink_set_address: %m");
+		}
 	}
 
 	return 0;
-- 
1.7.1

Re: [PATCH 2/3] Use %m instead of strerror() in syslog messages

[Marcel Holtmann]

[-- Attachment #1: Type: text/plain, Size: 458 bytes --]

Hi Remi,

> strerror() is not thread-safe, nor in the glibc implementation,
> nor in the POSIX specification. In my experience, it will infrequently
> crash when another thread uses locale data in any way, including
> string formatting and such...

I have no problem with this ...

> Since the Huawei driver uses thread, we might as well avoid strerror().

..., but what are you talking about? Where is it using threads?

Regards

Marcel

Re: [PATCH 2/3] Use %m instead of strerror() in syslog messages

[=?unknown-8bit?q?R=C3=A9mi?= Denis-Courmont]

[-- Attachment #1: Type: text/plain, Size: 659 bytes --]

On Thursday 25 November 2010 14:12:20 ext Marcel Holtmann, you wrote:
> Hi Remi,
> 
> > strerror() is not thread-safe, nor in the glibc implementation,
> > nor in the POSIX specification. In my experience, it will infrequently
> > crash when another thread uses locale data in any way, including
> > string formatting and such...
> 
> I have no problem with this ...
> 
> > Since the Huawei driver uses thread, we might as well avoid strerror().
> 
> ..., but what are you talking about? Where is it using threads?

Hmm, yeah. I misread the code. That comment is wrong.

-- 
Rémi Denis-Courmont
Nokia Devices R&D, Maemo Software, Helsinki

Re: [PATCH 2/3] Use %m instead of strerror() in syslog messages

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 1430 bytes --]

On 11/25/2010 04:51 AM, Rémi Denis-Courmont wrote:
> strerror() is not thread-safe, nor in the glibc implementation,
> nor in the POSIX specification. In my experience, it will infrequently
> crash when another thread uses locale data in any way, including
> string formatting and such...
> 
> Since the Huawei driver uses thread, we might as well avoid strerror().
> ---
>  drivers/isimodem/sim.c |    3 ++-
>  plugins/hfp.c          |    4 ++--
>  plugins/isigen.c       |   13 ++++++++-----
>  plugins/n900.c         |   14 +++++++-------
>  plugins/nokia-gpio.c   |   29 +++++++++++++++++------------
>  5 files changed, 36 insertions(+), 27 deletions(-)
> 

Since we do not use threads, do you still want this patch?  If you do,
can you please break this up into two patches, one for drivers/isimodem
and one for plugins?  We prefer patches to be split by directory.

<snip>

> @@ -765,7 +767,10 @@ int gpio_probe(GIsiModem *idx, unsigned addr, gpio_finished_cb_t cb, void *data)
>  	if (addr) {
>  		error = g_pn_netlink_set_address(idx, addr);
>  		if (error && error != -EEXIST)
> -			DBG("g_pn_netlink_set_address: %s", strerror(-error));
> +		{
> +			errno = -error;
> +			DBG("g_pn_netlink_set_address: %m");
> +		}
>  	}
>  
>  	return 0;

You might also want to fix the style violation above, the opening brace
should be on the same line as the if statement.

Regards,
-Denis

Re: [PATCH 2/3] Use %m instead of strerror() in syslog messages

[=?unknown-8bit?q?R=C3=A9mi?= Denis-Courmont]

[-- Attachment #1: Type: text/plain, Size: 1255 bytes --]

On Friday 26 November 2010 23:12:09 ext Denis Kenzior, you wrote:
> On 11/25/2010 04:51 AM, Rémi Denis-Courmont wrote:
> > strerror() is not thread-safe, nor in the glibc implementation,
> > nor in the POSIX specification. In my experience, it will infrequently
> > crash when another thread uses locale data in any way, including
> > string formatting and such...
> > 
> > Since the Huawei driver uses thread, we might as well avoid strerror().
> > ---
> > 
> >  drivers/isimodem/sim.c |    3 ++-
> >  plugins/hfp.c          |    4 ++--
> >  plugins/isigen.c       |   13 ++++++++-----
> >  plugins/n900.c         |   14 +++++++-------
> >  plugins/nokia-gpio.c   |   29 +++++++++++++++++------------
> >  5 files changed, 36 insertions(+), 27 deletions(-)
> 
> Since we do not use threads, do you still want this patch?  If you do,
> can you please break this up into two patches, one for drivers/isimodem
> and one for plugins?  We prefer patches to be split by directory.

Why do we initialize glib and dbus with thread support? If the plan is to 
smoothen the hypothetical use of threads in the future, then this patch should 
probably go in as well?

-- 
Rémi Denis-Courmont
Nokia Devices R&D, Maemo Software, Helsinki

Re: [PATCH 2/3] Use %m instead of strerror() in syslog messages

[Marcel Holtmann]

[-- Attachment #1: Type: text/plain, Size: 1447 bytes --]

Hi Remi,

> > > strerror() is not thread-safe, nor in the glibc implementation,
> > > nor in the POSIX specification. In my experience, it will infrequently
> > > crash when another thread uses locale data in any way, including
> > > string formatting and such...
> > > 
> > > Since the Huawei driver uses thread, we might as well avoid strerror().
> > > ---
> > > 
> > >  drivers/isimodem/sim.c |    3 ++-
> > >  plugins/hfp.c          |    4 ++--
> > >  plugins/isigen.c       |   13 ++++++++-----
> > >  plugins/n900.c         |   14 +++++++-------
> > >  plugins/nokia-gpio.c   |   29 +++++++++++++++++------------
> > >  5 files changed, 36 insertions(+), 27 deletions(-)
> > 
> > Since we do not use threads, do you still want this patch?  If you do,
> > can you please break this up into two patches, one for drivers/isimodem
> > and one for plugins?  We prefer patches to be split by directory.
> 
> Why do we initialize glib and dbus with thread support? If the plan is to 
> smoothen the hypothetical use of threads in the future, then this patch should 
> probably go in as well?

we do not initialize it with thread support, but in case a plugin
actually uses threads it could force it. This is common across BlueZ,
ConnMan and oFono and really just in case thing. In ConnMan we have this
case with the WiMAX plugin. For oFono we should hopefully never require
threading support.

Regards

Marcel

[PATCH 3/3] gisi: use strerror_r() instead of strerror()

[=?unknown-8bit?q?R=C3=A9mi?= Denis-Courmont]

[-- Attachment #1: Type: text/plain, Size: 1580 bytes --]

---
 gisi/netlink.c |    8 ++++++--
 gisi/server.c  |   11 ++++++++---
 2 files changed, 14 insertions(+), 5 deletions(-)

diff --git a/gisi/netlink.c b/gisi/netlink.c
index 44a4a8d..dcc322c 100644
--- a/gisi/netlink.c
+++ b/gisi/netlink.c
@@ -240,8 +240,12 @@ static gboolean g_pn_nl_process(GIOChannel *channel, GIOCondition cond,
 		case NLMSG_ERROR: {
 			struct nlmsgerr *err = NLMSG_DATA(nlh);
 			if (err->error)
-				g_printerr("Netlink error: %s",
-						strerror(-err->error));
+			{
+				char msg[256];
+
+				strerror_r(-err->error, msg, sizeof(msg));
+				g_printerr("Netlink error: %s", msg);
+			}
 			return TRUE;
 		}
 		case RTM_NEWADDR:
diff --git a/gisi/server.c b/gisi/server.c
index 159bb2d..df2e4eb 100644
--- a/gisi/server.c
+++ b/gisi/server.c
@@ -157,7 +157,10 @@ g_isi_server_add_name(GIsiServer *self)
 		return;
 
 	if (ioctl(self->fd, SIOCPNGETOBJECT, &object) < 0) {
-		g_warning("%s: %s", "ioctl(SIOCPNGETOBJECT)", strerror(errno));
+		char msg[256];
+
+		strerror_r(errno, msg, sizeof(msg));
+		g_warning("%s: %s", "ioctl(SIOCPNGETOBJECT)", msg);
 	} else {
 		struct sockaddr_pn spn = {
 			.spn_family = PF_PHONET,
@@ -173,8 +176,10 @@ g_isi_server_add_name(GIsiServer *self)
 
 		if (sendto(self->fd, req, sizeof(req), 0,
 				(void *)&spn, sizeof(spn)) != sizeof(req)) {
-			g_warning("%s: %s", "sendto(PN_NAMESERVICE)",
-				  strerror(errno));
+			char msg[256];
+
+			strerror_r(errno, msg, sizeof(msg));
+			g_warning("%s: %s", "sendto(PN_NAMESERVICE)", msg);
 		}
 	}
 }
-- 
1.7.1

Re: [PATCH 3/3] gisi: use strerror_r() instead of strerror()

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 1910 bytes --]

Hi Remi,

On 11/25/2010 04:51 AM, Rémi Denis-Courmont wrote:
> ---
>  gisi/netlink.c |    8 ++++++--
>  gisi/server.c  |   11 ++++++++---
>  2 files changed, 14 insertions(+), 5 deletions(-)
> 
> diff --git a/gisi/netlink.c b/gisi/netlink.c
> index 44a4a8d..dcc322c 100644
> --- a/gisi/netlink.c
> +++ b/gisi/netlink.c
> @@ -240,8 +240,12 @@ static gboolean g_pn_nl_process(GIOChannel *channel, GIOCondition cond,
>  		case NLMSG_ERROR: {
>  			struct nlmsgerr *err = NLMSG_DATA(nlh);
>  			if (err->error)
> -				g_printerr("Netlink error: %s",
> -						strerror(-err->error));
> +			{
> +				char msg[256];
> +
> +				strerror_r(-err->error, msg, sizeof(msg));
> +				g_printerr("Netlink error: %s", msg);
> +			}

So my question is why we're using g_printerr in the first place inside a
library.  Perhaps an approach similar to g_at_chat_set_debug is in order?

>  			return TRUE;
>  		}
>  		case RTM_NEWADDR:
> diff --git a/gisi/server.c b/gisi/server.c
> index 159bb2d..df2e4eb 100644
> --- a/gisi/server.c
> +++ b/gisi/server.c
> @@ -157,7 +157,10 @@ g_isi_server_add_name(GIsiServer *self)
>  		return;
>  
>  	if (ioctl(self->fd, SIOCPNGETOBJECT, &object) < 0) {
> -		g_warning("%s: %s", "ioctl(SIOCPNGETOBJECT)", strerror(errno));
> +		char msg[256];
> +
> +		strerror_r(errno, msg, sizeof(msg));
> +		g_warning("%s: %s", "ioctl(SIOCPNGETOBJECT)", msg);
>  	} else {
>  		struct sockaddr_pn spn = {
>  			.spn_family = PF_PHONET,
> @@ -173,8 +176,10 @@ g_isi_server_add_name(GIsiServer *self)
>  
>  		if (sendto(self->fd, req, sizeof(req), 0,
>  				(void *)&spn, sizeof(spn)) != sizeof(req)) {
> -			g_warning("%s: %s", "sendto(PN_NAMESERVICE)",
> -				  strerror(errno));
> +			char msg[256];
> +
> +			strerror_r(errno, msg, sizeof(msg));
> +			g_warning("%s: %s", "sendto(PN_NAMESERVICE)", msg);
>  		}
>  	}
>  }

Regards,
-Denis

Re: [PATCH 3/3] gisi: use strerror_r() instead of strerror()

[=?unknown-8bit?q?R=C3=A9mi?= Denis-Courmont]

[-- Attachment #1: Type: text/plain, Size: 373 bytes --]

On Friday 26 November 2010 23:07:29 ext Denis Kenzior, you wrote:
> So my question is why we're using g_printerr in the first place inside a
> library.  Perhaps an approach similar to g_at_chat_set_debug is in order?

I don't know. I guess I will wait for Aki's infamous rewritten gisi anyway.

-- 
Rémi Denis-Courmont
Nokia Devices R&D, Maemo Software, Helsinki

Re: [PATCH 1/3] Remove stray newlines in DBG and ofono log messages

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 809 bytes --]

Hi Rémi,

On 11/25/2010 04:51 AM, Rémi Denis-Courmont wrote:
> ---
>  drivers/isimodem/call-barring.c    |   10 +++++-----
>  drivers/isimodem/call-forwarding.c |    8 ++++----
>  drivers/isimodem/call-settings.c   |    8 ++++----
>  drivers/isimodem/cbs.c             |    2 +-
>  drivers/isimodem/sms.c             |    2 +-
>  drivers/stemodem/caif_rtnl.c       |    2 +-
>  drivers/stemodem/gprs-context.c    |    2 +-
>  plugins/isigen.c                   |    2 +-
>  src/gprs.c                         |    2 +-
>  src/sim.c                          |    2 +-
>  src/voicecall.c                    |    4 ++--
>  11 files changed, 22 insertions(+), 22 deletions(-)
> 

I applied this patch but broke it up into four:

- isimodem
- stemodem
- isigen
- core

Regards,
-Denis