Hi Giacinto,
so, first the documentation?
Correct. Whenever you touch something that affects the D-Bus API, it is
really preferable to have the D-Bus API changes in the preceding commit.
That way the reviewers can cross-reference what is being proposed to
the actual implementation. Anything that breaks the D-Bus API can also
be spotted much earlier.
oFono's D-Bus API is stable. That means we can add new things, but we
cannot change existing ones as that will break existing clients.
We also cannot break existing settings, as that would break oFono
installations on existing devices if they are upgraded.
This is a handicap that we have to live with right now.
<snip>
So there can be no unification with the GPRS naming now that the D-Bus
API is set?
I'm afraid not. But I'm not sure why this is a problem?
> +#define LTE_PROTO "Protocol"
> +#define LTE_USERNAME "Username"
> +#define LTE_PASSWORD "Password"
> +#define LTE_AUTH_METHOD "AuthenticationMethod"
>
> struct ofono_lte {
> const struct ofono_lte_driver *driver;
> @@ -50,13 +55,82 @@ struct ofono_lte {
> DBusMessage *pending;
> struct ofono_lte_default_attach_info pending_info;
> struct ofono_lte_default_attach_info info;
> + const char *prop_changed;
?? What memory location is this const char pointing to?
it is initialized to null with the containing structure.
That is not what I'm asking. This pointer points into data owned by
DBusMessage and the semantics of whether it is valid or not are not
enforced. Avoid that at all cost...
What about reading also the previous key?
Ideally you shouldn't change the key in the first place. But if you
are, then yes you need to be able to read legacy settings versions in
order to be backwards-compatible.
You do realize you're never actually calling into the driver method,
right? So none of these changes actually go out to the modem. Have
you
actually tested any of this?
Yes, it works. Actually the only call is when the APN is set, as
mentioned in the lte-api.txt.
No, it really doesn't...
And at that point all parameters are also set in the module.
It is not possible to set separately protocol and apn, and auth_method,
username, and password.
For ublox modules, the auth_method is also part of the APN name.
So we kept the call into the module when the APN is set, and previously
to it all other parameters are set.
You simply cannot do that. You cannot assume that the client knows how
your API is implemented underneath. So if they set the APN first and
then change the username, that has to work. This is why the
default_attach_info contains everything. If anything is changed the
entire structure is sent to the modem and every parameter is updated.
You have also mentioned that somewhere we should also verify that with
AUTH_NONE there are no user/pwd.
This also can only be verified at the end.
Any suggestions to improve this, given these limitations?
See above. Any parameter change has to trigger
set_default_attach_info() call. If something is invalid, then you have
to return a D-Bus error.
For example, if my method is set to 'none' and I try to do
LongTermEvolution.SetProperty("DefaultUsername", "foo") that should
return an error.
> + return dbus_message_ref(msg);;
> }
>
> static DBusMessage *lte_set_property(DBusConnection *conn,
> - DBusMessage *msg, void *data)
> + DBusMessage *msg,
void *data)
> {
> struct ofono_lte *lte = data;
> DBusMessageIter iter;
> DBusMessageIter var;
> const char *property;
> const char *str;
> + enum ofono_gprs_auth_method auth_method;
> + enum ofono_gprs_proto proto;
> +
> + if (lte->driver->set_default_attach_info == NULL)
> + return __ofono_error_not_implemented(msg);
> +
> + if (lte->pending)
> + return __ofono_error_busy(msg);
>
> if (!dbus_message_iter_init(msg, &iter))
> return __ofono_error_invalid_args(msg);
> @@ -192,13 +428,58 @@ static DBusMessage
*lte_set_property(DBusConnection *conn,
>
> dbus_message_iter_recurse(&iter, &var);
>
> - if (!strcmp(property, DEFAULT_APN_KEY)) {
> + lte->prop_changed=property;
> +
> + if (!strcmp(property, LTE_PROTO)) {
> +
> + if (dbus_message_iter_get_arg_type(&var) !=
DBUS_TYPE_STRING)
> + return __ofono_error_invalid_args(msg);
> +
> + dbus_message_iter_get_basic(&var, &str);
> +
> + if (gprs_proto_from_string(str, &proto))
> + return lte_set_proto(lte, conn, msg, proto);
The return from this callback is always supposed to be a method_return
or NULL if the method_return will be done asynchronously (in your case
always) You're somehow returning the method_call itself...
I am not sure I follow you, but you suggested to restructure the code,
so I will come back on this later.
You need to understand the semantics of GDBus. GDBUS_ASYNC_METHOD()
callback can only return the D-Bus method_return for message or NULL if
the method return will be generated asynchronously. You cannot return
the message itself. While this may 'work' the behavior is undefined.
> +struct ofono_modem *ofono_lte_get_modem(const struct ofono_lte *lte)
> +{
> + return __ofono_atom_get_modem(lte->atom);
> +}
> \ No newline at end of file
Fix this.
?
You have no newline after the last '}' and git complains. If git
complains I cannot apply the patch...
I agree it is unrelated, I will post a separate post, but I turn on
NEED_THREADS, and the build fails. Looking at the g_thread
documentations, nowadays it has to come out of the code:
|g_thread_init| has been deprecated since version 2.32 and should not be
used in newly-written code.
This function is no longer necessary. The GLib threading system is
automatically initialized at the start of your program.
Okay, please send this as a separate patch. We might need to remove
this configure option as well.
Regards,
-Denis
_______________________________________________
ofono mailing list
[email protected]
https://lists.ofono.org/mailman/listinfo/ofono
