kasprintf combines kmalloc and sprintf, and takes care of the size calculation itself.
The semantic patch that makes this change is as follows:
// <smpl> @@ expression a,flag; expression list args; statement S; @@
a = - (kmalloc|kzalloc)(...,flag) + kasprintf(flag,args) <... when != a if (a == NULL || ...) S ...> - sprintf(a,args); // </smpl>
Signed-off-by: Himangi Saraogi himangi774@gmail.com Acked-by: Julia Lawall julia.lawall@lip6.fr --- net/batman-adv/sysfs.c | 24 ++++++++---------------- 1 file changed, 8 insertions(+), 16 deletions(-)
diff --git a/net/batman-adv/sysfs.c b/net/batman-adv/sysfs.c index fc47baa..f40cb04 100644 --- a/net/batman-adv/sysfs.c +++ b/net/batman-adv/sysfs.c @@ -900,32 +900,24 @@ int batadv_throw_uevent(struct batadv_priv *bat_priv, enum batadv_uev_type type,
bat_kobj = &bat_priv->soft_iface->dev.kobj;
- uevent_env[0] = kmalloc(strlen(BATADV_UEV_TYPE_VAR) + - strlen(batadv_uev_type_str[type]) + 1, - GFP_ATOMIC); + uevent_env[0] = kasprintf(GFP_ATOMIC, + "%s%s", BATADV_UEV_TYPE_VAR, + batadv_uev_type_str[type]); if (!uevent_env[0]) goto out;
- sprintf(uevent_env[0], "%s%s", BATADV_UEV_TYPE_VAR, - batadv_uev_type_str[type]); - - uevent_env[1] = kmalloc(strlen(BATADV_UEV_ACTION_VAR) + - strlen(batadv_uev_action_str[action]) + 1, - GFP_ATOMIC); + uevent_env[1] = kasprintf(GFP_ATOMIC, + "%s%s", BATADV_UEV_ACTION_VAR, + batadv_uev_action_str[action]); if (!uevent_env[1]) goto out;
- sprintf(uevent_env[1], "%s%s", BATADV_UEV_ACTION_VAR, - batadv_uev_action_str[action]); - /* If the event is DEL, ignore the data field */ if (action != BATADV_UEV_DEL) { - uevent_env[2] = kmalloc(strlen(BATADV_UEV_DATA_VAR) + - strlen(data) + 1, GFP_ATOMIC); + uevent_env[2] = kasprintf(GFP_ATOMIC, + "%s%s", BATADV_UEV_DATA_VAR, data); if (!uevent_env[2]) goto out; - - sprintf(uevent_env[2], "%s%s", BATADV_UEV_DATA_VAR, data); }
ret = kobject_uevent_env(bat_kobj, KOBJ_CHANGE, uevent_env);
On Sun, 2014-06-29 at 00:06 +0530, Himangi Saraogi wrote:
kasprintf combines kmalloc and sprintf, and takes care of the size calculation itself.
Nice. A small conversion to remove unnecessary initializations, avoid calling kfree with known NULL pointers, and save a few bytes of code space woud be: --- net/batman-adv/sysfs.c | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-)
diff --git a/net/batman-adv/sysfs.c b/net/batman-adv/sysfs.c index f40cb04..d6fba94 100644 --- a/net/batman-adv/sysfs.c +++ b/net/batman-adv/sysfs.c @@ -896,7 +896,7 @@ int batadv_throw_uevent(struct batadv_priv *bat_priv, enum batadv_uev_type type, { int ret = -ENOMEM; struct kobject *bat_kobj; - char *uevent_env[4] = { NULL, NULL, NULL, NULL }; + char *uevent_env[3];
bat_kobj = &bat_priv->soft_iface->dev.kobj;
@@ -910,22 +910,23 @@ int batadv_throw_uevent(struct batadv_priv *bat_priv, enum batadv_uev_type type, "%s%s", BATADV_UEV_ACTION_VAR, batadv_uev_action_str[action]); if (!uevent_env[1]) - goto out; + goto out0;
/* If the event is DEL, ignore the data field */ if (action != BATADV_UEV_DEL) { uevent_env[2] = kasprintf(GFP_ATOMIC, "%s%s", BATADV_UEV_DATA_VAR, data); if (!uevent_env[2]) - goto out; + goto out1; }
ret = kobject_uevent_env(bat_kobj, KOBJ_CHANGE, uevent_env); -out: - kfree(uevent_env[0]); - kfree(uevent_env[1]); kfree(uevent_env[2]); - +out1: + kfree(uevent_env[1]); +out0: + kfree(uevent_env[0]); +out: if (ret) batadv_dbg(BATADV_DBG_BATMAN, bat_priv, "Impossible to send uevent for (%s,%s,%s) event (err: %d)\n",
Hi all,
On 28/06/14 21:13, Joe Perches wrote:
diff --git a/net/batman-adv/sysfs.c b/net/batman-adv/sysfs.c index f40cb04..d6fba94 100644 --- a/net/batman-adv/sysfs.c +++ b/net/batman-adv/sysfs.c @@ -896,7 +896,7 @@ int batadv_throw_uevent(struct batadv_priv *bat_priv, enum batadv_uev_type type, { int ret = -ENOMEM; struct kobject *bat_kobj;
- char *uevent_env[4] = { NULL, NULL, NULL, NULL };
- char *uevent_env[3];
Joe, why are you shortening this? kobject_uevent_env() expect a NULL-terminating array (that is the forth cell).
...
ret = kobject_uevent_env(bat_kobj, KOBJ_CHANGE, uevent_env);
And how is this change reducing the code space?
For what concerns the labels, we use this pattern mostly all over the code: one single label/exit-point with the related NULL checks. Do you think that we can improve something by changing this? (I am not talking about the fastpath here).
Cheers,
On Sat, 2014-06-28 at 21:49 +0200, Antonio Quartulli wrote:
Hi all,
On 28/06/14 21:13, Joe Perches wrote:
diff --git a/net/batman-adv/sysfs.c b/net/batman-adv/sysfs.c
[]
@@ -896,7 +896,7 @@ int batadv_throw_uevent(struct batadv_priv *bat_priv, enum batadv_uev_type type, { int ret = -ENOMEM; struct kobject *bat_kobj;
- char *uevent_env[4] = { NULL, NULL, NULL, NULL };
- char *uevent_env[3];
Joe, why are you shortening this? kobject_uevent_env() expect a NULL-terminating array (that is the forth cell).
Hi Antonio, sorry, I didn't know about the last NULL being required. It looked to me more like an oversight in the code instead of a required NULL.
And how is this change reducing the code space?
Removing unnecessary initializations reduces object code size.
For what concerns the labels, we use this pattern mostly all over the code: one single label/exit-point with the related NULL checks. Do you think that we can improve something by changing this? (I am not talking about the fastpath here).
Not calling known unnecessary kfree calls helps a little. Certainly, it'd be more valuable in any fast path area.
Other than that, it was an unsigned suggestion not a formal patch submission.
Ignore it or improve it as you see fit.
cheers, Joe
Maybe: --- net/batman-adv/sysfs.c | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-)
diff --git a/net/batman-adv/sysfs.c b/net/batman-adv/sysfs.c index f40cb04..90c245e 100644 --- a/net/batman-adv/sysfs.c +++ b/net/batman-adv/sysfs.c @@ -896,7 +896,7 @@ int batadv_throw_uevent(struct batadv_priv *bat_priv, enum batadv_uev_type type, { int ret = -ENOMEM; struct kobject *bat_kobj; - char *uevent_env[4] = { NULL, NULL, NULL, NULL }; + char *uevent_env[4];
bat_kobj = &bat_priv->soft_iface->dev.kobj;
@@ -910,22 +910,26 @@ int batadv_throw_uevent(struct batadv_priv *bat_priv, enum batadv_uev_type type, "%s%s", BATADV_UEV_ACTION_VAR, batadv_uev_action_str[action]); if (!uevent_env[1]) - goto out; + goto out0;
/* If the event is DEL, ignore the data field */ if (action != BATADV_UEV_DEL) { uevent_env[2] = kasprintf(GFP_ATOMIC, "%s%s", BATADV_UEV_DATA_VAR, data); if (!uevent_env[2]) - goto out; + goto out1; }
+ uevent_env[3] = NULL; + ret = kobject_uevent_env(bat_kobj, KOBJ_CHANGE, uevent_env); -out: - kfree(uevent_env[0]); - kfree(uevent_env[1]); - kfree(uevent_env[2]);
+ kfree(uevent_env[2]); +out1: + kfree(uevent_env[1]); +out0: + kfree(uevent_env[0]); +out: if (ret) batadv_dbg(BATADV_DBG_BATMAN, bat_priv, "Impossible to send uevent for (%s,%s,%s) event (err: %d)\n",
On Sat, 28 Jun 2014, Antonio Quartulli wrote:
Hi all,
On 28/06/14 21:13, Joe Perches wrote:
diff --git a/net/batman-adv/sysfs.c b/net/batman-adv/sysfs.c index f40cb04..d6fba94 100644 --- a/net/batman-adv/sysfs.c +++ b/net/batman-adv/sysfs.c @@ -896,7 +896,7 @@ int batadv_throw_uevent(struct batadv_priv *bat_priv, enum batadv_uev_type type, { int ret = -ENOMEM; struct kobject *bat_kobj;
- char *uevent_env[4] = { NULL, NULL, NULL, NULL };
- char *uevent_env[3];
Joe, why are you shortening this? kobject_uevent_env() expect a NULL-terminating array (that is the forth cell).
...
ret = kobject_uevent_env(bat_kobj, KOBJ_CHANGE, uevent_env);
And how is this change reducing the code space?
For what concerns the labels, we use this pattern mostly all over the code: one single label/exit-point with the related NULL checks. Do you think that we can improve something by changing this? (I am not talking about the fastpath here).
Most of the kernel uses specific labels for each possible failure.
From my selfish point of view, it makes the code easier to analyze and
understand, because what is done at the exit label is only what needs to be done.
julia
From: Himangi Saraogi himangi774@gmail.com Date: Sun, 29 Jun 2014 00:06:29 +0530
kasprintf combines kmalloc and sprintf, and takes care of the size calculation itself.
The semantic patch that makes this change is as follows:
...
Signed-off-by: Himangi Saraogi himangi774@gmail.com Acked-by: Julia Lawall julia.lawall@lip6.fr
Applied, thanks.
b.a.t.m.a.n@lists.open-mesh.org
-
Antonio Quartulli -
David Miller -
Himangi Saraogi -
Joe Perches -
Julia Lawall