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

[Marcel Holtmann <marcel@holtmann.org>]

[-- Attachment #1: Type: text/plain, Size: 1249 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.

Regards

Marcel