RE: [PATCH 2/2] Fix: Check if interface exists before creating it.

[Marcel Holtmann <marcel@holtmann.org>]

[-- Attachment #1: Type: text/plain, Size: 1692 bytes --]

Hi Sjur,

> >> diff --git a/drivers/stemodem/gprs-context.c
> >> b/drivers/stemodem/gprs-context.c index c178fb6..79e697b 100644
> >> --- a/drivers/stemodem/gprs-context.c
> >> +++ b/drivers/stemodem/gprs-context.c
> >> @@ -187,9 +187,11 @@ static gboolean caif_if_create(const char
> >>  	*interface, unsigned int connid)  		return FALSE; }
> >> 
> >> -	if (ioctl(s, SIOCCAIFNETNEW, &ifr) < 0) {
> >> -		ofono_debug("Failed to create IP interface for CAIF");
> >> -		return FALSE;
> >> +	if (ioctl(s, SIOCGIFINDEX, &ifr) != 0) {
> >> +		if (ioctl(s, SIOCCAIFNETNEW, &ifr) < 0) {
> >> +			ofono_debug("Failed to create IP interface for CAIF");
> >> +			return FALSE; +		}
> >>  	}
> >> 
> >>  	return TRUE;
> > 
> > just doing it like this would be better:
> > 
> > 	/* Check if interface exists */
> > 	if (ioctl(s, SIOCGIFINDEX, &ifr) == 0)
> > 		return TRUE;
> > 
> > 	if (ioctl(s, SIOCCAIFNETNEW, &ifr) < 0) {
> > 		...
> > 		return FALSE;
> > 	}
> > 
> > 	return TRUE;
> > 
> > We are not a big fan complicated if clauses if they can be avoid with
> > a simple goto or just a return. 
> > 
> > Also I think we need to check errno value here. Since potentially the
> > ioctl can fail for other reasons. And maybe extending CAIF with a
> > proper way of checking for an existing interface might be better.  
> 
> Do you have anything particular in mind here? 
> Maybe CAIF could simply return EEXIST from SIOCCAIFNETNEW if the interface
> already exists? In this case we could return TRUE anyway.

if it isn't that doing already, then it should be doing it. And yes,
just returning TRUE in that case seems fine to me.

Regards

Marcel