Hi Mohamed,
> Add initial vpn support.
I said that I gonna merge it as is, but I have some extra comments on
the patch that I prefer to get fixed before the merge.
It is nothing major actually, mainly some coding style stuff that would
be pointless to fix up after merging the patch.
> +enum connman_provider_error {
> + CONNMAN_PROVIDER_ERROR_UNKNOWN = 0,
> + CONNMAN_PROVIDER_ERROR_CONNECT_FAILED = 1,
> + CONNMAN_PROVIDER_ERROR_NO_CONNECTION = 2,
> +};
We are not using the NO_CONNECTION error right no. So leave it out until
it is actually needed.
> +
> +
> +struct connman_provider;
> +
> +struct connman_provider *connman_provider_ref(struct connman_provider
> *provider);
> +void connman_provider_unref(struct connman_provider *provider);
> +
> +int connman_provider_set_string(struct connman_provider *provider,
> + const char *key, const char *value);
> +const char *connman_provider_get_string(struct connman_provider *provider,
> + const char *key);
> +
> +int connman_provider_set_connected(struct connman_provider *provider,
> + connman_bool_t connected);
> +
> +void connman_provider_set_index(struct connman_provider *provider, int
> index);
> +int connman_provider_get_index(struct connman_provider *provider);
> +
> +void connman_provider_set_data(struct connman_provider *provider, void
> *data);
> +void *connman_provider_get_data(struct connman_provider *provider);
> +
> +void connman_provider_set_gateway(struct connman_provider *provider,
> + const char *gateway);
> +void connman_provider_set_address(struct connman_provider *provider,
> + const char *address);
> +void connman_provider_set_netmask(struct connman_provider *provider,
> + const char *netmask);
> +void connman_provider_set_dns(struct connman_provider *provider,
> + const char *dns);
> +void connman_provider_set_domain(struct connman_provider *provider,
> + const char *domain);
> +
> +
> +static gboolean provider_exit = FALSE;
Explain to me why this is needed. Seems like a race condition somewhere
and an ugly hack around it?
> +struct connman_provider {
> + struct connman_element element;
> + char *identifier;
> + char *path;
> + enum connman_provider_state state;
> + enum connman_provider_error error;
> + GTimeVal modified;
Leave the modified field out of this. Since we are not sorting the
provider list, we don't need this detail.
> + char *name;
> + char *type;
> + char *dns;
> + char *domain;
> + DBusMessage *pending;
> + guint timeout;
> + struct connman_provider_driver *driver;
> + void *driver_data;
> +};
> +
> +static struct connman_provider *connman_provider_create(const char *name);
> +static int connman_provider_disconnect(struct connman_provider *provider);
I do dislike forward declarations. Why are they needed?
> +
> +static const char *state2string(enum connman_provider_state state)
> +{
> + switch (state) {
> + case CONNMAN_PROVIDER_STATE_UNKNOWN:
> + break;
> + case CONNMAN_PROVIDER_STATE_IDLE:
> + return "idle";
> + case CONNMAN_PROVIDER_STATE_CONNECT:
> + return "connecting";
Please call this one "connect".
> +void __connman_provider_list(DBusMessageIter *iter)
> +{
> + g_hash_table_foreach(provider_hash, append_path, iter);
> +}
> +
> +
Please stop these double empty lines. We don't do that.
> +static void connman_provider_setup_vpn_ipv4(struct connman_provider
> *provider,
> + struct connman_element *element)
> +{
> +
> + if ((element == NULL) || (provider == NULL))
> + return;
The extra ( ) is pointless. Just do if (element == NULL && ...).
> +void connman_provider_unref(struct connman_provider *provider)
> +{
> + connman_element_unref(&provider->element);
> +}
> +
> +
And again double empty lines.
> + if ((driver->probe != NULL) && (driver->probe(provider) == 0)) {
> + provider->driver = driver;
> + break;
Here we have the pointless ( ) again. Really? What are you trying to
protect here. The C arithmetic is pretty clear and this is not needed.
> + dbus_message_iter_open_container(&entry, DBUS_TYPE_VARIANT,
> + DBUS_TYPE_STRING_AS_STRING, &value);
> + dbus_message_iter_append_basic(&value, DBUS_TYPE_STRING, &str);
> + dbus_message_iter_close_container(&entry, &value);
> +
> + g_dbus_send_message(connection, signal);
> +}
> +
> +
Again?
> +int __connman_provider_indicate_state(struct connman_provider *provider,
> + enum connman_provider_state state)
> +{
> +
> + DBG("provider %p state %d", provider, state);
And what is this extra empty line before DBG for? Remove it.
> +
> + if (provider_exit)
> + return -EINVAL;
> +
> + if (provider == NULL)
> + return -EINVAL;
> +
> + if (provider->state == state)
> + return -EALREADY;
> +
> + if (provider->state == CONNMAN_PROVIDER_STATE_FAILURE &&
> + state == CONNMAN_PROVIDER_STATE_IDLE)
> + return -EINVAL;
> +
> + if (provider->state == CONNMAN_PROVIDER_STATE_IDLE &&
> + state == CONNMAN_PROVIDER_STATE_DISCONNECT)
> + return -EINVAL;
> +
> + if (state == CONNMAN_PROVIDER_STATE_IDLE &&
> + provider->state != CONNMAN_PROVIDER_STATE_DISCONNECT) {
> + provider->state = CONNMAN_PROVIDER_STATE_DISCONNECT;
> + state_changed(provider);
> +
> + connman_provider_disconnect(provider);
> + }
Please turn this into a switch statement. All the ifs make it
unreadable.
> +
> + provider->state = state;
> + state_changed(provider);
> +
> + if (state == CONNMAN_PROVIDER_STATE_READY) {
> + reply_pending(provider, 0);
> + g_get_current_time(&provider->modified);
> +
> + }
You have an extra empty line here before } for no reason. Remove that.
> + return __connman_provider_indicate_state(provider,
> + CONNMAN_PROVIDER_STATE_FAILURE);
> +}
> +
> +
And again. Seriously, I don't even see a pattern here.
> +static gboolean connect_timeout(gpointer user_data)
> +{
> + struct connman_provider *provider = user_data;
> +
> + DBG("provider %p", provider);
> +
> + provider->timeout = 0;
> +
> +
And what are these two for?
> + if (provider->pending != NULL) {
> + DBusMessage *reply;
> +
> + reply = __connman_error_operation_timeout(provider->pending);
> + if (reply != NULL)
> + g_dbus_send_message(connection, reply);
> +
> + dbus_message_unref(provider->pending);
> + provider->pending = NULL;
> + }
> +
> + __connman_provider_indicate_error(provider,
> + CONNMAN_PROVIDER_ERROR_CONNECT_FAILED);
> +
> + return FALSE;
> +}
> +
> +
And some more.
> + if ((provider->driver != NULL) && (provider->driver->connect != NULL))
> + err = provider->driver->connect(provider);
The extra ( ) have to go away.
> + else
> + return -EOPNOTSUPP;
> +
> + if (err < 0) {
> + if (err != -EINPROGRESS)
> + return err;
> +
> + provider->timeout = g_timeout_add_seconds(60,
> + connect_timeout, provider);
> +
> + __connman_provider_indicate_state(provider,
> + CONNMAN_PROVIDER_STATE_CONNECT);
> +
> +
And some more extra empty lines on sale ;)
> + return -EINPROGRESS;
> + }
> +
> + return 0;
> +}
> +
> +
Some more.
> +static int connman_provider_disconnect(struct connman_provider *provider)
> +{
> + int err;
> +
> + DBG("provider %p", provider);
> +
> + reply_pending(provider, ECONNABORTED);
> +
> + if ((provider->driver != NULL) &&
> + (provider->driver->disconnect != NULL))
> + err = provider->driver->disconnect(provider);
The extra ( ) have to go.
> + g_hash_table_remove(provider_hash, path);
> + return 0;
> +}
> +
> +
And again.
> + if (path != NULL) {
> + g_dbus_unregister_interface(connection, path,
> + CONNMAN_PROVIDER_INTERFACE);
> + g_free(path);
> + }
> +
> +
Found some more.
> + if (provider_exit)
> + if ((provider->driver != NULL) && (provider->driver->remove))
> + provider->driver->remove(provider);
Extra ( ).
> +
> + connman_element_unregister(&provider->element);
> + connman_provider_unref(provider);
> +}
> +
> +
More empty lines.
> + provider->identifier = NULL;
> + provider->path = NULL;
> +
> + provider->pending = NULL;
> +
Useless empty line.
> +}
> +
> +
And again.
> + g_dbus_register_interface(connection, provider->path,
> + CONNMAN_PROVIDER_INTERFACE,
> + provider_methods, provider_signals,
> + NULL, provider, NULL);
> +
> + return 0;
> +}
> +
> +
I am getting tried of this now.
> +failed:
> + if (provider != NULL && created == TRUE) {
> + DBG("can not connect delete provider");
> + connman_provider_unref(provider);
> + }
> +
> + return err;
> +}
> +
> +
It never ends, does it?
> +void connman_provider_set_data(struct connman_provider *provider, void *data)
> +{
> + provider->driver_data = data;
> +}
> +
> +
And I did think it was over ...
> +void connman_provider_driver_unregister(struct connman_provider_driver
> *driver)
> +{
> + DBG("driver %p name %s", driver, driver->name);
> +
> + driver_list = g_slist_remove(driver_list, driver);
> +
> +}
What is that empty line for. Remove it.
Regards
Marcel
_______________________________________________
connman mailing list
[email protected]
http://lists.connman.net/listinfo/connman
