Thanks for your time on this Marcel.
Jay
> Hi Jay,
>
>> +if OPENPLUG
>> +if OPENPLUG_BUILTIN
>> +builtin_modules += openplug
>> +builtin_sources += plugins/openplug.c
>> +builtin_cflags += $(optelephony_CFLAGS)
>> +builtin_libadd += $(optelephony_LIBS)
>> +else
>> +plugin_LTLIBRARIES += plugins/openplug.la
>> +plugin_objects += $(plugins_openplug_la_OBJECTS)
>> +plugins_openplug_la_LDFLAGS = $(plugin_ldflags) $(optelephony_LIBS)
>> +plugins_openplug_la_CFLAGS = $(plugin_cflags) $(optelephony_CFLAGS)
>> +endif
>> +endif
>> +
>
> I prefer to not allow builtin plugin here. It depends on external
> libraries and it makes no sense from packaging point of view anyway.
>
>> +AC_ARG_ENABLE(openplug,
>> + AC_HELP_STRING([--enable-openplug], [enable OPENPLUG support]),
>> + [enable_openplug=${enableval}],
>> [enable_openplug="no"])
>> +AM_CONDITIONAL(OPENPLUG, test "${enable_openplug}" != "no")
>> +AM_CONDITIONAL(OPENPLUG_BUILTIN, test "${enable_openplug}" = "builtin")
>> +if test "$enable_openplug" = "yes" ; then
>
> So even if you allow builtin plugins then this check is wrong. It needs
> to be != "no" like above.
>
> So better remove the builtin support since that was clearly untested.
>
>> +#ifdef HAVE_CONFIG_H
>> +#include <config.h>
>> +#endif
>> +#include <unistd.h>
>
> If you follow the coding style from the other files, then we always
> include an empty line after the HAVE_CONFIG_H block.
>
>> +#define CONNMAN_API_SUBJECT_TO_CHANGE
>> +#include <connman/plugin.h>
>> +#include <connman/device.h>
>> +#include <connman/inet.h>
>> +#include <connman/log.h>
>> +#include <connman/resolver.h>
>> +#include <connman/notifier.h>
>> +
>> +#include <optelephony/settings.h>
>> +#include <optelephony/dcm.h>
>> +#include <optelephony/telephony_server.h>
>> +#include <optelephony/tapi.h>
>> +
>> +static dcm_connection_t dcm_connection;
>> +static dcm_ip_config_t dcm_ip_config;
>> +
>> +static unsigned char netmask2prefixlen(const char *netmask)
>> +{
>> + unsigned char bits = 0;
>> + in_addr_t mask = inet_network(netmask);
>> + in_addr_t host = ~mask;
>> +
>> + /* a valid netmask must be 2^n - 1 */
>> + if ((host & (host + 1)) != 0)
>> + return -1;
>> +
>> + for (; mask; mask <<= 1)
>> + ++bits;
>> +
>> + return bits;
>> +}
>
> If you copy the code, please copy it and keep the formatting in place.
>
> For now placing the code here is fine, but in the long run, it might be
> better to put it somewhere more generic. Maybe src/inet.c.
>
>> +static int setup_interface(struct connman_device *device)
>> +{
>> + int index;
>> + struct connman_ipaddress *ipaddress;
>> + struct in_addr gateway_addr;
>> +
>> + index = connman_device_get_index(device);
>> +
>> + if (dcm_connection != NULL)
>> + dcm_get_ip_config(dcm_connection, &dcm_ip_config);
>> +
>> + DBG("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) {
>> + DBG(" No gateway specified, defaulting to 0.0.0.0\n");
>> + strcpy(dcm_ip_config.gateway_addr, dcm_ip_config.ip_addr);
>> + }
>
> Use g_strcmp0 instead of strcmp.
>
>> +
>> + ipaddress = connman_ipaddress_alloc();
>> + ipaddress->local = dcm_ip_config.ip_addr;
>> + ipaddress->peer = 0;
>> + ipaddress->prefixlen = netmask2prefixlen("255.255.255.0");
>> + ipaddress->broadcast = "0.0.0.0";
>> +
>> + connman_inet_set_address(index, ipaddress);
>> +
>> + inet_aton(dcm_ip_config.gateway_addr, &gateway_addr);
>> + connman_inet_set_gateway(index, gateway_addr);
>> +
>
> The plugin is directly configuring the interface. That is not a good
> idea. Look at how the oFono plugin does it. It is a better solution.
>
>> +/**
>> + * 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;
>
> I don't understand this. Why are you doing it. It is pointless. Just use
> event directly.
>
>> + struct connman_device *device = (struct connman_device *) user_data;
>
> No cast of a void pointer is needed. Just assign user_data directly.
>
>> + default:
>> + break;
>> + }
>> + }
>> + return;
>
> This return is pointless. Remove it.
>
>> +static int dcm_disconnect(struct connman_device *device)
>> +{
>> +
>
> No empty line here.
>
>> +static int dcm_connect(struct connman_device *device)
>> +{
>> +
>
> No empty lines here.
>> + char *profile = NULL;
>> + dcm_return_t dcm_ret;
>> +
>> + if (connman_settings_get_current_profile_name(&profile) ==
>> + SETTINGS_RETURN_SUCCESS) {
>
> What is wrong with just doing.
>
> if (... != SETTINGS_RETURN_SUCCESS)
> return -1;
>
>> +
>> + if (dcm_create_connection(&dcm_connection) ==
>> + dcm_return_success) {
>> +
>> + if (dcm_connection_event_register(dcm_connection,
>> + telephony_plugin_event_cb,
>> + device) == dcm_return_success) {
>> +
>
> And then of course.
>
> if (... != dcm_return_success)
> return -1;
>
>> + dcm_ret = dcm_activate_connection(
>> + dcm_connection,
>> + profile, 1);
>> + if (dcm_ret == dcm_return_success) {
>> + free(profile);
>> + return TRUE;
>> + } else {
>> + if (dcm_ret == dcm_return_failure)
>> + DBG("failure profile %s",
>> + profile);
>> + }
>> + dcm_connection_event_unregister(
>> + dcm_connection);
>> + }
>> + dcm_destroy_connection(dcm_connection);
>> + }
>> + free(profile);
>> + }
>
> After these changes, then the function becomes actually readable and
> will not hurt my brain every time I look at it.
>
>> + return TRUE;
>
> This function return int and not boolean.
>
>> +}
>> +
>> +static int telephony_probe(struct connman_device *device)
>> +{
>> + const char *interface_name;
>> +
>> + DBG("");
>> + interface_name = connman_device_get_name(device);
>> + DBG("Device name : %s", interface_name);
>
> You can't just assume that get_name gives you the interface name.
>
>> +
>> + connman_device_set_powered(device, TRUE);
>
> The probe function should not use set_powered. That is not a plugin
> policy. You need to handle this properly.
>
>> + connman_device_set_mode(device, CONNMAN_DEVICE_MODE_TRANSPORT_IP);
>> + connman_device_set_carrier(device, TRUE);
>
> Both options are gone now. Every connman_device needs a connman_network.
>
>> + connman_device_set_connected(device, FALSE);
>> + return 0;
>> +}
>> +
>> +static void telephony_remove(struct connman_device *device)
>> +{
>> + DBG("");
>> +}
>> +
>> +static int telephony_disconnect(struct connman_device *device)
>> +{
>> + DBG("");
>> + dcm_disconnect(device);
>> + return 0;
>> +}
>> +
>> +static int telephony_connect(struct connman_device *device)
>> +{
>> + int index;
>> +
>> + DBG("");
>> + index = connman_device_get_index(device);
>> + connman_inet_ifup(index);
>> + dcm_connect(device);
>> + return 0;
>> +}
>
> The connect and disconnect callbacks are gone now. You need a
> connman_network.
>
>> +static int telephony_enable(struct connman_device *device)
>> +{
>> + DBG("");
>> + return 0;
>> +}
>> +
>> +static int telephony_disable(struct connman_device *device)
>> +{
>> + DBG("");
>> + return 0;
>> +}
>
> These two have to be implemented. And enable/disable the telephony
> system. That is how flight mode is controlled.
>
>> +static int telephony_init(void)
>> +{
>> + DBG("");
>> + return connman_device_driver_register(&openplug_driver);
>> +}
>> +
>> +static void telephony_exit(void)
>> +{
>> + DBG("");
>> + connman_device_driver_unregister(&openplug_driver);
>> +}
>
> Get rid of the debug prints in these two. The core daemon will tell you
> and there can't go anything wrong here anyway.
>
>> diff --git a/src/inet.c b/src/inet.c
>> index ac68698..847e188 100644
>> --- a/src/inet.c
>> +++ b/src/inet.c
>> @@ -417,7 +417,8 @@ enum connman_device_type
>> __connman_inet_get_device_type(int index)
>> else
>> devtype = CONNMAN_DEVICE_TYPE_ETHERNET;
>> } else if (type == ARPHRD_NONE) {
>> - if (g_str_has_prefix(devname, "hso") == TRUE)
>> + if ((g_str_has_prefix(devname, "hso") == TRUE) ||
>> + (g_str_has_prefix(devname, "gtm") == TRUE))
>> devtype = CONNMAN_DEVICE_TYPE_HSO;
>> }
>
> This is a bad idea since you depend on the telephony daemon running. I
> mentioned that in the other review. You have to add code that does
> runtime detection of the telephony daemon. We do the same for oFono and
> BlueZ and you have to do this, too.
>
> Regards
>
> Marcel
>
>
> _______________________________________________
> connman mailing list
> [email protected]
> http://lists.connman.net/listinfo/connman
>
_______________________________________________
connman mailing list
[email protected]
http://lists.connman.net/listinfo/connman
