Hi James,
> From: Jay Freyensee <jpfre...@fedora-5150-smashcake.(none)>
you need to edit your .gitconfig, because your sender address is weird
and I can accept patches like that. Also when using git send-email, you
better use linux.intel.com.
Applying: openplug submission
/data/devel/connman-openplug/.git/rebase-apply/patch:69: new blank line at EOF.
+
fatal: 1 line adds whitespace errors.
Patch failed at 0001 openplug submission
When you have resolved this problem run "git am --resolved".
If you would prefer to skip this patch, instead run "git am --skip".
To restore the original branch and stop patching run "git am --abort".
And you patch doesn't apply when using git am. Please fix that.
> +#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;
Do we really only have one DCM connection and configuration for the
whole system. What happens if multiple devices are used. Like for
example phones with Dual SIM card slots/modems.
> + 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);
There is something wrong here. Either missing { } or you meant something
different. Also where is the error handling in case dcm_connection is
actually NULL? If it can't be NULL, then don't bother checking it.
> +/**
> + * Connection event callback from dcm, in the telephony context.
> + */
> +static void openplug_plugin_event_cb(dcm_connection_event_t *event,
> + const void *user_data)
> +{
> + dcm_connection_event_t *evt = (dcm_connection_event_t *) event;
I asked this before. Why this cast? And why at all. evt and event are
exactly the same. Just use event.
> + struct connman_device *device = (struct connman_device *) user_data;
This case is not needed. void pointers assignment is just fine.
> +
> + if (evt == NULL)
> + return;
> +
> + if (evt->ret == dcm_return_success) {
> + switch (evt->status) {
> + case dcm_connection_connecting:
> + break;
This needs to like this:
if (evt->ret != dcm_return_success)
return;
switch (event->status) {
...
Avoids one extra line of indentation that makes the code more harder to
read and my brain hort.
> +static int dcm_disconnect(struct connman_device *device)
> +{
> +
> + if (dcm_connection != NULL) {
if (dcm_connection == NULL)
return 0;
> + if (dcm_deactivate_connection(dcm_connection) !=
> + dcm_return_success) {
> + DBG("dcm connection deactivate failed\n");
> + dcm_destroy_connection(dcm_connection);
> + return -1;
> + }
if (dcm_deactivate_connection(...) == dcm_return_success)
return 0;
> + DBG("dcm connection deactivated\n");
> + }
> + return 0;
> +}
You really need to get this sorted out. It makes the code a lot easier
to read and to follow the failure cases.
> +static int dcm_connect(struct connman_device *device)
> +{
> +
> + char *profile = NULL;
> + dcm_return_t dcm_ret;
> +
> + if (connman_settings_get_current_profile_name(&profile) ==
> + SETTINGS_RETURN_SUCCESS) {
> +
The connman_settings_get_current_profile_name() function does not exist.
Have you actually tested this against latest ConnMan GIT tree before
submitting it? And ...
if (connman_...() != SETTINGS_RETURN_SUCCESS)
return TRUE.
This is essential. You need to get the massive indentation mess out of
this patch. Otherwise I will not be able to apply. These could all
simple if-return statements.
> + if (dcm_create_connection(&dcm_connection) ==
> + dcm_return_success) {
> +
> + if (dcm_connection_event_register(dcm_connection,
> + openplug_plugin_event_cb,
> + device) == dcm_return_success) {
> +
> + 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);
> + }
> + return TRUE;
> +}
Think about it and re-write it. It is too complex.
> +static int openplug_probe(struct connman_device *device)
> +{
> + struct telephony_data *data;
> + int index;
> + const char *interface_name;
> +
> + DBG("device: %p", device);
> +
> +
> + data = g_try_new0(struct telephony_data, 1);
> + if (data == NULL)
> + return -ENOMEM;
> +
> +
> + connman_device_set_data(device, data);
> + index = connman_device_get_index(device);
> +
> + interface_name = connman_device_get_name(device);
> + DBG("Device name : %s", interface_name);
This debug looks a bit pointless and more like a leftover.
> +static int openplug_init(void)
> +{
> + int err;
> +
> + DBG("openplug_init");
Actually DBG("") is just fine here. It includes the function name
anyway.
> +
> + err = connman_network_driver_register(&openplug_network_driver);
> + if (err < 0)
> + return err;
> +
> + err = connman_device_driver_register(&openplug_modem_driver);
> +
> + if (err < 0) {
> + connman_network_driver_unregister(&openplug_network_driver);
> + return err;
> + }
> +
> + return 0;
> +}
This still doesn't handle termination and restart of the OpenPlug
telephony server. That is a requirement. Looks at the oFono, BlueZ and
even WiFi plugins on how they handle this.
> +static void openplug_exit(void)
> +{
> + DBG("openplug_exit");
Same as above.
> + connman_device_driver_unregister(&openplug_modem_driver);
> + connman_network_driver_register(&openplug_network_driver);
> +}
> +
> +CONNMAN_PLUGIN_DEFINE(openplug, "Telephony interface plugin",
> + CONNMAN_VERSION,
> + CONNMAN_PLUGIN_PRIORITY_DEFAULT,
> + openplug_init, openplug_exit)
> +
Regards
Marcel
_______________________________________________
connman mailing list
[email protected]
http://lists.connman.net/listinfo/connman
