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 <[email protected]>
Acked-by: Julia Lawall <[email protected]>
---
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);
--
1.9.1
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,
--
Antonio Quartulli
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 <[email protected]>
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 <[email protected]>
> Acked-by: Julia Lawall <[email protected]>
Applied, thanks.