On Wed, Sep 19, 2018 at 6:30 PM Denis Kenzior <denk...@gmail.com> wrote:
> 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.
>
Hi Denis,
I had a look at this all, and I have a problem. I cannot check the
parameters as they are entered one by one.
Example: if I blank user/pwd when the authentication is changed to NONE,
then if changed again to CHAP, the module will reject it.
Shall I store the parameters, or keep them also in case of error?
Another way would be to have a SetParameters() function, and set them all
together, including the APN, and not allowing writing them separately,
apart from the APN which already exists.
I don't really like it, either.
Or introduce an atom function that is called before modem->set_online(true)?
thanks,
Giacinto
>
> >
> > 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
ofono@ofono.org
https://lists.ofono.org/mailman/listinfo/ofono
