Hi James,
first things first, I don't mind having patches as attachment as
long as
they are flawless and I don't have to comment on them. Otherwise I
would
prefer you use git send-email and you can just use linux.intel.com as
SMTP server. That is what others are doing quite successfully.
-src_connmand_LDADD = $(builtin_libadd) \
- @GLIB_LIBS@ @DBUS_LIBS@ @UDEV_LIBS@
-ldl
+src_connmand_LDADD = $(builtin_libadd) @GLIB_LIBS@ @GTHREAD_LIBS@ \
+ @DBUS_LIBS@
@UDEV_LIBS@ -ldl
Why do we need GTHREAD_LIBS here. Essentially this plugin should be
talking via D-Bus to the telephony server, right?
And just linking against pthread etc. is racy as hell. You must make
sure that GLib and D-Bus are initialized so they can handle threaded
mainloops.
In addition we are doing this differently with extending GLIB_LIBS
with
GTHREAD_LIBS if threading is really needed. I still doubt that.
+if OP3G
+if OP3G_BUILTIN
+builtin_modules += op3g
+builtin_sources += plugins/op3g.c
+else
+plugin_LTLIBRARIES += plugins/op3g.la
+plugin_objects += $(plugins_op3g_la_OBJECTS)
+plugins_op3g_la_LDFLAGS = $(plugin_ldflags) $(optelephony_LIBS)
+plugins_op3g_la_CFLAGS = $(plugin_cflags) $(optelephony_CFLAGS)
-lpthread
+endif
+endif
Don't add support for builtin here. It makes no sense since it is an
external dependency that we can't resolve properly.
Also location is either before IWMX or after IOSPM, but not in the
section of standard plugins due to its external dependency.
+AC_ARG_ENABLE(op3g,
+ AC_HELP_STRING([--enable-op3g], [enable OP3G support]),
+ [enable_op3g=${enableval}],
[enable_op3g="no"])
+AM_CONDITIONAL(OP3G, test "${enable_op3g}" != "no")
+AM_CONDITIONAL(OP3G_BUILTIN, test "${enable_op3g}" = "builtin")
+if test "$enable_op3g" = "yes" ; then
+ PKG_CHECK_MODULES([optelephony], [optelephony], dummy=yes,
+ AC_MSG_ERROR(optelephony is required))
+ AC_SUBST(optelephony_CFLAGS)
+ AC_SUBST(optelephony_LIBS)
+fi
I don't like op3g. Call it either openplug or optelephony.
And FLAGS and LIBS variables are all uppercase.
+#ifdef HAVE_CONFIG_H
+#include <config.h>
+#endif
Empty line after this one.
+#include <unistd.h>
+#include <string.h>
+#include <stdlib.h>
+#include <stdio.h>
+#include <unistd.h>
+#include <errno.h>
+#include <signal.h>
+#include <netinet/in.h>
+#include <net/if.h>
+#include <sys/ioctl.h>
+#include <sys/socket.h>
+#include <net/ethernet.h>
+
+#define CONNMAN_API_SUBJECT_TO_CHANGE
+// #define L_CONNMAN_VERSION "0.45"
+#define L_CONNMAN_VERSION CONNMAN_VERSION
Remove this L_CONNMAN_* non-sense. It is pointless.
+// this will be used if MODEM_INTERFACE is not found in terminal.opt
+#define DEVICE_NAME
"gtm0" // cdk
This is not acceptable at all. No hard-coding of interface names in
the
driver.
+#define TERMINAL_OPT_FILEPATH
"/etc/open-plug/my-devices/Terminal.opt"
What is this for? I don't like this at all.
+#define TIMEOUT_VALUE (40)
+#define MAX_LINE_LENGTH 80
+#define MAX_NAME_LENGTH 20
+char ifname[MAX_NAME_LENGTH];
What is this doing. I am pretty sure this is unneeded. In addition why
export ifname. That should be at least static. However this raises the
question how to support multiple interface. Don't do stuff like this.
+dcm_connection_t dcm_connection;
+dcm_ip_config_t dcm_ip_config;
+struct connman_device *device;
+int not_received;
+int in_flight=TRUE;
+pthread_t thread1, thread2;
Exported variables. No way, they all need to be static at minimum.
And I high doubt we need threads here. If we do something in the
library
API seems major misdesigned.
+// function prototypes in the actual plugin that depend on each
other
+static char *inet_index2ident(int index, const char *prefix);
+static int get_interface_name_from_file(char *ifn);
+static void telephony_offline_mode(connman_bool_t enabled);
+static int hso_disconnect();
+static int telephony_disable(struct connman_device *device);
+static int telephony_enable(struct connman_device *device);
+static void* timeout_loop();
+int telephony_probe(struct connman_device *device);
No prototypes. Order your code in a way that forward declarations are
not needed. And if and only if they are really needed, keep it to a
bare
minimum. If you need more than one, they your code is too complex.
+static char *inet_index2ident(int index, const char *prefix)
+{
There should be a ConnMan wide public function for this. And I don't
even thing the prefix part is needed and connman_inet_ifname should
work
good enohgh.
+enum telephony_service_state_t {
+ TELEPHONY_STATE_DISABLED = 0,
+ TELEPHONY_STATE_IDLE = 1,
+ TELEPHONY_STATE_CONNECTING = 2,
+ TELEPHONY_STATE_CONNECTED = 3,
+};
+
+enum telephony_service_state_t state;
Again, exported, public state. And one global state. I repeat that is
not how things are done. They have to support multiple interfaces at
the
same time.
+#ifndef IFF_LOWER_UP
+#define IFF_LOWER_UP 0x10000
+#endif
Move this to the top after the includes like all the other plugins do.
Having this somewhere in the middle of the code makes it hard to
maintain.
+static int get_interface_name_from_file(char *ifn) {
+ char line[MAX_LINE_LENGTH];
+ FILE *fr;
+
+ fr = fopen (TERMINAL_OPT_FILEPATH, "rt"); /* open the file for
reading */
+
+ strcpy(ifn,"");
+
+ if (fr == NULL ) /* Could not open file */
+ {
+ DBG("---------------> %s\n", TERMINAL_OPT_FILEPATH);
+ return -1;
+ }
+
+ while(fgets(line, MAX_LINE_LENGTH, fr) != NULL)
+ {
+ if (strncmp(line, "ModemInterfaceName",
strlen("ModemInterfaceName") )==0) {
+ DBG("---------------> Interface name found in
Terminal.opt\n");
+ strncpy(ifn,&line[strlen("ModemInterfaceName")+1],
10);
+ ifn[strlen(ifn)-1]='\0';
+ break;
+ }
+ }
+ fclose(fr); /* close the file prior to exiting the routine */
+ return 0;
+}
First of all the coding style here is totally off. There is no excuse
for this. The whole plugin should follow the same coding style to
begin
with and it should follow the project coding style which is mostly
kernel coding style.
And second, why do we have to read the interface name from a
configuration file?
+static void telephony_offline_mode(connman_bool_t enabled)
+{
+ int ret = 0;
+ if (enabled) {
+ DBG("---------------> ask telephony server to shut
down radio %d\n", enabled);
+ ret =
tel_set_phone_functionality(telc_phone_fun_noTxRx);
+ if (ret != telc_success)
+ {
+ DBG("---------------> Error while enabling
offline mode\n");
+ } else {
+ DBG("---------------> Flight mode is ON,
modem
radio is switched off\n");
+ in_flight = TRUE;
+ }
+ }
+ else {
+ DBG("---------------> state %d\n", state);
+ if (in_flight) {
+ DBG("---------------> ask telephony server to
power on radio %d\n", enabled);
+ ret =
tel_set_phone_functionality(telc_phone_fun_full);
+ if (ret != telc_success)
+ {
+ DBG("---------------> Error while
disabling flight mode\n");
+ } else {
+ DBG("---------------> modem radio is
switched on\n");
+ in_flight = FALSE;
+ }
+ }
+ }
+}
Where should I begin here. So all this "-------" debug has to go. That
makes the whole code unreadable.
Also please actually think about what this code is suppose to be doing
and then re-write it. It is plain ugly.
+int telephony_probe(struct connman_device *device)
+{
Why is this not static?
+ char *ident = NULL;
+ int index = -1;
+
+ struct telephony_data *telephony;
Move this one as first variable declaration.
+
+ DBG("device %p", device);
+
+ telephony = g_try_new0(struct telephony_data, 1);
+ if (telephony == NULL)
+ return -ENOMEM;
+
+ DBG("---------------> Telephony plugin probe %p\n", device);
And more of this stupid debug. Remove them.
+ telephony->flags = 0;
+
+ get_interface_name_from_file((char *)&ifname);
I don't know why this cast is needed. And if it would be needed, then
please like this (char *) &ifname.
However then on the other hand the static interface names are wrong to
begin with.
+ if (strcmp(ifname,"")==0) {
Coding style. if (strcmp(ifname, "") == 0) {
+ DBG("---------------> Modem network interface not
found in Terminal.opt, defaulting to %s\n", DEVICE_NAME);
+ strcpy(ifname,DEVICE_NAME);
Coding style, strcpy(ifname, DEVICE_NAME);
+ }
+
+ DBG("---------------> Telephony plugin probe -%s-\n",
ifname);
+
+ connman_device_set_interface(device, ifname, NULL);
+ index = connman_inet_ifindex(ifname);
+
+ ident = inet_index2ident(index, NULL);
+ if (ident) {
+ connman_device_set_ident(device, ident);
+ }
For single instruction no { }
+
+ connman_device_set_index(device,index);
+ connman_device_set_data(device, telephony);
+ telephony->index = connman_device_get_index(device);
+
+ telephony->watch =
connman_rtnl_add_newlink_watch(telephony->index,
+ op_telephony_newlink,
device);
+
+ state = TELEPHONY_STATE_IDLE;
Global state. Not really good. Especially pointless since you do
have a
data pointer here.
+static int hso_connect()
+{
+ int index=-1;
+ struct connman_ipaddress *ipaddress = NULL;
+ struct in_addr local_gateway_addr;
Coding style, tabs vs. spaces.
And I don't like if you initialize variables. That is a bad habit to
hide actual errors.
+ if (dcm_connection!=NULL) {
Coding style.
+ dcm_get_ip_config(dcm_connection,&dcm_ip_config);
+ DBG("---------------> dcm_get_ip_config interface:
\"%
s\"\n", dcm_ip_config.interface_name);
+ index =
connman_inet_ifindex(dcm_ip_config.interface_name);
+ }
+
+ if (strlen(dcm_ip_config.ip_addr)==0) {
Coding style.
+ DBG("---------------> dcm_get_ip_config failed,
cannot
connect\n");
+ }
+
+ if (index<0) {
Coding style.
+ DBG("---------------> Simulator mode\n");
+ DBG("---------------> Custom configuration for
interface required: IFNAME: %s IP ADDR: %s DNS1: %s DNS2: %s GW: %s
\n",
+ dcm_ip_config.interface_name,
dcm_ip_config.ip_addr, dcm_ip_config.dns1_addr,
+ dcm_ip_config.dns2_addr,
+ dcm_ip_config.gateway_addr);
+ return -1;
+ }
+
+ DBG("---------------> Custom configuration for interface
required: IFNAME: %s IP ADDR: %s DNS1: %s DNS2: %s GW: %s\n",
+ dcm_ip_config.interface_name,
dcm_ip_config.ip_addr, dcm_ip_config.dns1_addr,
+ dcm_ip_config.dns2_addr,
+ dcm_ip_config.gateway_addr);
+
+ if (strcmp(dcm_ip_config.gateway_addr,"0.0.0.0") == 0) {
Coding style.
+ DBG("---------------> No gateway specified by
operator, gateway is 0.0.0.0\n");
+ strcpy(dcm_ip_config.gateway_addr,
dcm_ip_config.ip_addr);
+ }
+
+ connman_inet_ifup(index);
+ connman_device_set_index(device, index);
+
connman_device_set_interface(device,dcm_ip_config.interface_name,
NULL);
Coding style.
+
+ ipaddress = connman_ipaddress_alloc();
+ ipaddress->local = dcm_ip_config.ip_addr;
+ ipaddress->peer = 0;
+ ipaddress->prefixlen = 0;
+ ipaddress->broadcast = "0.0.0.0";
+
+ inet_aton(dcm_ip_config.gateway_addr, &local_gateway_addr);
+
+ connman_inet_set_address(index, ipaddress);
+ connman_inet_set_gateway(index, local_gateway_addr);
+
+ connman_resolver_append(dcm_ip_config.interface_name, NULL,
+ dcm_ip_config.dns1_addr);
+ connman_resolver_append(dcm_ip_config.interface_name, NULL,
+ dcm_ip_config.dns2_addr);
+
+ state = TELEPHONY_STATE_CONNECTED;
+ // We want to keep this in here for now to show
+ // it is understood to free the structure but
+ // connman core dumps when it is tried.
+ // connman_ipaddress_free(ipaddress);
+ return 0;
+}
+
+
+static int hso_disconnect()
Function name is misleading.
+{
+ if (dcm_connection!=NULL) {
Coding style.
+
+ if(dcm_destroy_connection(dcm_connection) !=
dcm_return_success)
Coding style.
+ {
+ DBG("---------------> issue when destroying
dcm connection\n");
+ dcm_connection = NULL;
+ return -1;
+ }
+
+ if(dcm_connection_event_unregister(dcm_connection)!=
dcm_return_success)
+ {
Coding style.
+ DBG("---------------> issue when
unregistering
to dcm connection events\n");
+ dcm_deactivate_connection(dcm_connection);
+ dcm_destroy_connection(dcm_connection);
+ dcm_connection = NULL;
+ return -1;
+ }
+ }
+ state = TELEPHONY_STATE_IDLE;
+
+ return 0;
+}
+
+
+
+/**
Too many empty lines. One is enough.
+ * Connection event callback from dcm, in the telephony context.
+ */
+static void telephony_plugin_event_cb(dcm_connection_event_t *event,
const void *user_data)
+{
+ dcm_connection_event_t *evt = (dcm_connection_event_t *) event;
And why is this cast plus assignment needed. Looks totally pointless
here.
+
+ if(evt == NULL)
+ return;
Coding style.
+
+ if (evt->ret == dcm_return_success)
+ {
With if and switch the { is on the same line.
+ switch (evt->status)
+ {
+ case dcm_connection_connecting:
+ DBG("---------------> Connecting...\n");
+ break;
+
+ case dcm_connection_authenticating:
+ DBG("---------------> Authenticating...\n");
+ break;
+
+ case dcm_connection_authenticated:
+ DBG("---------------> Registering...\n");
+ break;
All useless.
+
+ case dcm_connection_connected:
+ DBG("---------------> Connected...\n");
+ not_received = FALSE;
+ hso_connect();
+ break;
+
+ case dcm_connection_disconnected:
+ DBG("---------------> Disconnected...\n");
+ hso_disconnect();
+ break;
Spaces vs. tabs.
+
+ default:
+ break;
+ }
+ }
+ else
+ {
This is } else {
+ DBG("---------------> Failure...\n");
+ hso_disconnect();
And spaces again.
+static int telephony_disconnect(struct connman_device *device)
+{
+ int index;
+ DBG("---------------> %p\n", device);
+
+ index = connman_device_get_index(device);
+ connman_inet_ifdown(index);
+
+ if (state==TELEPHONY_STATE_IDLE) {
+ DBG("---------------> service is not connected, cannot
disconnect\n");
+ return -1;
+ }
Coding style.
+ if (thread1>0) pthread_join( thread1, NULL);
Coding style and why threads?
+
+ if (dcm_connection != NULL)
+ {
+ connman_resolver_remove_all
(dcm_ip_config.interface_name);
+ if(dcm_deactivate_connection(dcm_connection) !=
dcm_return_success)
+ {
+ DBG("---------------> dcm connection
deactivate failed\n");
+ state=TELEPHONY_STATE_IDLE;
+ dcm_destroy_connection(dcm_connection);
+ dcm_connection = NULL;
+ return -1;
+ }
+ DBG("---------------> dcm connection deactivated\n");
+ } else DBG("---------------> No dcm connection to disconnect
\n");
+
+ if (device!=NULL)
+ return connman_device_set_connected(device, FALSE);
+ else return -1;
+}
Full of coding style violations.
+static void* timeout_loop()
+{
+ int i=0;
+ int timeoutElapsed=FALSE;
+ not_received = TRUE;
+
+ while (not_received && !timeoutElapsed)
+ {
+ DBG("---------------> -");
+ sleep(1);
+ i++;
+ if (i>=TIMEOUT_VALUE) timeoutElapsed = TRUE;
+ }
+ if (timeoutElapsed) {
+ DBG("---------------> Timout elapsed, disconnecting
device for safety");
+ if (device!=NULL) telephony_disconnect(device);
+ state=TELEPHONY_STATE_IDLE;
+ } else {
+ DBG("---------------> Event received in time,
connected");
+ }
+ return NULL;
+}
Oh hell no. We are not doing such stupid crap. And especially not
spawning a thread to have it sleep(1).
+static int telephony_connect(struct connman_device *device)
+{
+ dcm_return_t dcm_ret = dcm_return_failure;
+ char *profile = NULL;
No variable initialization until really required.
+
+ DBG("---------------> %p\n", device);
+ if (state==TELEPHONY_STATE_DISABLED) {
+ DBG("---------------> Cannot connect while service is
disabled
\n");
+ return -1;
+ }
+
+ if (state==TELEPHONY_STATE_CONNECTING ||
state==TELEPHONY_STATE_CONNECTED) {
+ DBG("---------------> service is already connecting/
connected,
cannot connect\n");
+ return -1;
+ }
+ state = TELEPHONY_STATE_CONNECTING;
+
+ if (connman_settings_get_current_profile_name(&profile) ==
SETTINGS_RETURN_SUCCESS)
+ {
+ if (dcm_create_connection(&dcm_connection) ==
dcm_return_success)
+ {
+ if (dcm_connection_event_register(dcm_connection,
telephony_plugin_event_cb, device) == dcm_return_success)
+ {
+ pthread_create( &thread1, NULL, timeout_loop, NULL);
+ sleep(1); // let time for thread
timeout_loop to be scheduled
+ dcm_ret = dcm_activate_connection(dcm_connection,
profile,1);
+
+ if (dcm_ret == dcm_return_success)
+ {
+ DBG("---------------> Waiting for modem
connection
event\n");
+ free(profile);
+
+ return TRUE;
+ }
+ else if (dcm_ret == dcm_return_failure)
+ DBG("---------------> Dcm_activate_connection
failed for profile %s", profile);
+ else
+ {
+ DBG("---------------> Dcm_activate_connection
failed because connection is busy");
+ // dcm_activate_connection failed
+ }
+ dcm_connection_event_unregister(dcm_connection);
+ state = TELEPHONY_STATE_IDLE;
+ }
+
+ // dcm_connection_event_register or
dcm_activate_connection failed
+ dcm_destroy_connection(dcm_connection);
+ dcm_connection = NULL;
+ }
+
+ free(profile);
+ }
+ else
+ {
+ //No Connman profile selected
+ DBG("---------------> No default connman profile set or
telephony server is down");
+ state = TELEPHONY_STATE_IDLE;
+ }
+
+ return 0;
+}
I am not even gonna read this part until the coding style has been
fixed
and the massive debug crap has been removed.
+static int telephony_enable(struct connman_device *device)
+{
+ struct telephony_data *telephony = NULL;
Really? You init the variable and then assign it one statement later.
+ DBG("---------------> %p\n", device);
+
+ telephony = connman_device_get_data(device);
+ return connman_inet_ifup(telephony->index);
+}
+static struct connman_device_driver telephony_driver = {
+ .name = DEVICE_NAME,
This is not the device interface name. This would be "optelephony" or
something similar.
+ .type = CONNMAN_DEVICE_TYPE_HSO,
Mis-using the HSO type is not really gonna fly.
+ .probe = telephony_probe,
+ .remove = telephony_remove,
+ .enable = telephony_enable,
+ .disable = telephony_disable,
+ .connect = telephony_connect,
+ .disconnect = telephony_disconnect,
+};
+
+
+static int telephony_init(void)
+{
+ int err;
+ DBG("---------------> Telephony plugin init");
+
+
+ device = connman_device_create(DEVICE_NAME,
CONNMAN_DEVICE_TYPE_HSO);
+
+ if (device == NULL)
+ return -EIO;
+
+ connman_device_set_mode(device,
CONNMAN_DEVICE_MODE_TRANSPORT_IP);
+ connman_device_register(device);
+
+
+ err = connman_device_driver_register(&telephony_driver);
+
+ return err;
+}
And this won't work. You can not create a device when loading the
plugin. You need to detect if the OP telephony server is running or
not
and then create the device.
Also in addition, I would prefer having a device for the telephony
server and then a network for the actual PDP context. That makes the
whole structure way cleaner. And allows us to simply the device
handling. Since in the future even an Ethernet device will need to
have
a network (aka cable) and can't get IP configuration directly anymore.
Regards
Marcel
_______________________________________________
connman mailing list
[email protected]
http://lists.connman.net/listinfo/connman
