Thanks for the comments Marcel. I did use linux.intel.com as
"-smtp-server linux.intel.com". I'm not sure how that happened.
I don't think Open Plug's telephony server was designed with more than one
SIM card per system in mind. But I'll have to ask.
Yes, this all compiled as of today at about 1pm. So if it is mentioned
that a function does not exist, then it still existed when I submitted it.
I do not just blindly submit a patch without at least making sure
everything compiles one last time.
Thanks for the review,
Jay
> 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
>
_______________________________________________
connman mailing list
[email protected]
http://lists.connman.net/listinfo/connman
