RE: [PATCH 2/2] Fix: Check if interface exists before creating it.
Hi Marcel.
Marcel Holtmann wrote:
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.
BR/Sjur
___
ofono mailing list
ofono@ofono.org
http://lists.ofono.org/listinfo/ofono
Re: [PATCH 2/2] Fix: Check if interface exists before creating it.
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
___
ofono mailing list
ofono@ofono.org
http://lists.ofono.org/listinfo/ofono
RE: [PATCH 2/2] Fix: Check if interface exists before creating it.
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
___
ofono mailing list
ofono@ofono.org
http://lists.ofono.org/listinfo/ofono
