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