Re: [B.A.T.M.A.N.] [PATCH 1/3] batman-adv: add wrapper function to throw uevent in userspace

[Antonio Quartulli <ordex@autistici.org>]

On gio, mag 05, 2011 at 03:34:24 +0200, Andrew Lunn wrote:
> > +	uevent_env[0] = kmalloc(strlen("BATTYPE=") +
> > +				strlen(uev_type_str[type]) + 1,
> > +				GFP_ATOMIC);
> > +	if (!uevent_env[0])
> > +		goto out;
> > +
> > +	sprintf(uevent_env[0], "BATTYPE=%s", uev_type_str[type]);
> 
> Hi Antonio
> 

Hi Andrew,

> I don't particularly like having BATTYPE= twice, once in the kmalloc
> and a second time in the sprintf. Maybe somebody will decide that
> BATUTYPE is a better name, change the sprintf, forget about the
> kmalloc, and overflow the allocated memory by one byte. snprintf will
> prevent the corruption. I've made this sort of stupid error myself,
> and it takes longer to debug than to write a bit more defensive code
> which prevents the error.

I definitely agree. I thin that using a define like
#define UEV_TYPE_VAR "BATTYPE="
would be more elegant.
In this case I can avoid to use snprintf. Do you agree on this?

> > +	/* If the event is DEL, ignore the data field */
> > +	if (action == UEV_DEL)
> > +		goto throw;
> 
> I would replace this goto with a plain if statement. 

Mh..ok. I abused of this if->goto style even if not really needed

> > +	if (ret)
> > +		bat_dbg(DBG_BATMAN, bat_priv, "Impossible to send "
> > +			"uevent for (%s,%s,%s) event\n",
> > +			uev_type_str[type], uev_action_str[action],
> > +			(action == UEV_DEL ? "NULL" : data));
> 
> The value of ret could be interesting here, especially if kobject_uevent_env() failed.

Mh, Ok. I can print it into the message. Is there a function in the kernel to
transform it in a proper string?

Thank you very much.

Regards,

-- 
Antonio Quartulli

..each of us alone is worth nothing..
Ernesto "Che" Guevara