Search Linux Wireless

Re: [PATCH] mac80211_hwsim: configfs support

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





--- Em seg, 8/12/08, Jouni Malinen <j@xxxxx> escreveu:

> De: Jouni Malinen <j@xxxxx>
> Assunto: Re: [PATCH] mac80211_hwsim: configfs support
> Para: "Igor Trindade Oliveira" <igor_trindade@xxxxxxxxxxxx>
> Cc: johannes@xxxxxxxxxxxxxxxx, linux-wireless@xxxxxxxxxxxxxxx, linville@xxxxxxxxxxxxx
> Data: Segunda-feira, 8 de Dezembro de 2008, 11:05
> On Fri, Dec 05, 2008 at 01:37:48PM -0800, Igor Trindade
> Oliveira wrote:
> 
> > Mac80211_hwsim: Support to configfs operations for
> create a new radio
> 
> The change in general looks reasonable, but I have couple
> of comments
> about the details.
> 
> Could you please also update
> Documentation/networking/mac80211_hwsim/
> README to replace the kernel module parameter with
> description on how to
> add virtual radios?
Ok i will go to do that.

> 
> > +++
> wireless-testing/drivers/net/wireless/mac80211_hwsim.c	2008-12-02
> 06:21:56.000000000 -0400
> > @@ -140,10 +138,15 @@ struct mac80211_hwsim_data {
> >  		PS_DISABLED, PS_ENABLED, PS_AUTO_POLL,
> PS_MANUAL_POLL
> >  	} ps;
> >  	bool ps_poll_pending;
> > -	struct dentry *debugfs;
> > -	struct dentry *debugfs_ps;
> > +	struct config_item item;
> >  };
> >  
> > +struct mac80211_hwsim_attr {
> > +	struct configfs_attribute attr;
> > +	 ssize_t(*show) (struct mac80211_hwsim_data *hwsim,
> char *buf);
> > +	 ssize_t(*store) (struct mac80211_hwsim_data *hwsim,
> > +			  const char *buf, ssize_t count);
> 
> There is extra space (<TAB><SPACE>ssize_t) in
> indentation here. As far
> as function pointer style is concerned, my preference would
> be to move
> the space to be before the variable name: ssize_t
> (*show)(struct..).
> 
> > +static struct configfs_atribbute
> *mac80211_hwsim_attrs[] = {
> 
> Has this been even compile tested? I would assume no, since
> that struct
> name has two typos in it..
> 
> > +/*
> > + * now we just have one attribute with this function
> we can
> > + * add new attributes in mac80211_hwsim_attrs and do
> not change
> > + * others parts of the code
> > + */
> 
> Is that comment specific to this function or is it just
> description of
> what the change in whole is doing? Is it describing the
> change, it
> should be in commit message, not in the source code. If it
> describe this
> particular function, I would suggest somewhat different
> wording to make
> that cleaner. It could also be moved above
> mac80211_hwsim_attrs
> initialization if it is describing how new attributes are
> added.
It's a comment to describing what we need to do for add new attributes.

> 
> > @@ -875,3 +993,4 @@ static void __exit
> exit_mac80211_hwsim(v
> >  
> >  module_init(init_mac80211_hwsim);
> >  module_exit(exit_mac80211_hwsim);
> > +
> 
> Please don't add unnecessary (and unwanted) whitespace
> changes.
> 

Ok. i will to go to fix that.
> -- 
> Jouni Malinen                                           
> PGP id EFC895FA
> --
> To unsubscribe from this list: send the line
> "unsubscribe linux-wireless" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at 
> http://vger.kernel.org/majordomo-info.html

cheers,

igor


      Veja quais são os assuntos do momento no Yahoo! +Buscados
http://br.maisbuscados.yahoo.com

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[Index of Archives]     [Linux Host AP]     [ATH6KL]     [Linux Bluetooth]     [Linux Netdev]     [Kernel Newbies]     [Linux Kernel]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Samba]     [Device Mapper]
  Powered by Linux