Re: [PATCH] wireless: consolidate on a single escape_essid implementation

[Dan Williams <dcbw@redhat.com>]

On Thu, 2008-09-25 at 06:46 +0200, Johannes Berg wrote:
> On Wed, 2008-09-24 at 18:15 -0400, John W. Linville wrote:
> > This is also an excuse to create the long rumored lib80211 module...
> 
> Let's also take the chance to clean up the mess Intel has, once again,
> created here.
> 
> >  		switch (info_element->id) {
> >  		case MFIE_TYPE_SSID:
> > -			if (ieee80211_is_empty_essid(info_element->data,
> > -						     info_element->len)) {
> > +			if (is_empty_essid(info_element->data,
> > +					   info_element->len)) {
> >  				network->flags |= NETWORK_EMPTY_ESSID;
> >  				break;
> >  			}
> 
> You can remove the whole NETWORK_EMPTY_ESSID flag and this whole test;
> the code slightly below needs to take care to actually escape the SSID
> for the debug output.
> 
> > @@ -1411,7 +1412,7 @@ static int ieee80211_handle_assoc_resp(struct ieee80211_device *ieee, struct iee
> >  			network->mode |= IEEE_B;
> >  	}
> >  
> > -	if (ieee80211_is_empty_essid(network->ssid, network->ssid_len))
> > +	if (is_empty_essid(network->ssid, network->ssid_len))
> >  		network->flags |= NETWORK_EMPTY_ESSID;
> 
> drop the whole if
>  
> >  
> > -	if (ieee80211_is_empty_essid(network->ssid, network->ssid_len))
> > +	if (is_empty_essid(network->ssid, network->ssid_len))
> >  		network->flags |= NETWORK_EMPTY_ESSID;
> 
> ditto.
> 
> that leaves once place using the NETWORK_EMPTY_ESSID flag, in
> ieee80211_wx.c:
> 
>         /* Add the ESSID */
>         iwe.cmd = SIOCGIWESSID;
>         iwe.u.data.flags = 1;
>         if (network->flags & NETWORK_EMPTY_ESSID) {
>                 iwe.u.data.length = sizeof("<hidden>");
>                 start = iwe_stream_add_point(info, start, stop,
>                                              &iwe, "<hidden>");
>         } else {
>                 iwe.u.data.length = min(network->ssid_len, (u8) 32);
>                 start = iwe_stream_add_point(info, start, stop,
>                                              &iwe, network->ssid);
>         }
> 
> which is of course *completely* *wrong*. There's no excuse for messing
> up the values passed to userspace like that.
> 
> > +const char *escape_essid(const char *essid, u8 essid_len)
> > +{
> > +	static char escaped[IW_ESSID_MAX_SIZE * 2 + 1];
> > +	const char *s = essid;
> > +	char *d = escaped;
> > +
> > +	if (is_empty_essid(essid, essid_len)) {
> > +		memcpy(escaped, "<hidden>", sizeof("<hidden>"));
> > +		return escaped;
> > +	}
> 
> Once you've done the above, is_empty_essid is only used here. I'll leave
> it to you whether you want to print <hidden> or not, I'd prefer if the
> function was to just escape the ASCII-NULs as normal since then you can
> actually distinguish the various forms of hidden SSIDs which might help.
> Maybe that needs quotes around the escaped SSID then.
> 
> > +	essid_len = min(essid_len, (u8) IW_ESSID_MAX_SIZE);
> > +	while (essid_len--) {
> > +		if (*s == '\0') {
> > +			*d++ = '\\';
> > +			*d++ = '0';
> > +			s++;
> > +		} else {
> > +			*d++ = *s++;
> > +		}
> > +	}
> > +	*d = '\0';
> > +	return escaped;
> > +}
> > +EXPORT_SYMBOL(escape_essid);
> 
> Also, we could take the chance to stop making Jean's mistake in calling
> this thing the _E_SSID, there's no such thing, it's Jean's invention,
> based on his misguided thought that this might somehow be less confusing
> to the user.

+1

Dan