Hi Denis,
On Fri, Oct 12, 2018 at 8:10 PM Denis Kenzior <[email protected]> wrote:
>
> Hi Giacinto,
>
> > static void at_lte_set_default_attach_info(const struct ofono_lte *lte,
> > const struct ofono_lte_default_attach_info *info,
> > ofono_lte_cb_t cb, void *data)
> > {
> > struct lte_driver_data *ldd = ofono_lte_get_data(lte);
> > char buf[32 + OFONO_GPRS_MAX_APN_LENGTH + 1];
> > - struct cb_data *cbd = cb_data_new(cb, data);
> > + struct lte_callbackdata *cbd = g_new0(struct lte_callbackdata ,1);
> > + const char *proto;
> > + size_t len;
> >
> > + cbd->cb = cb;
> > + cbd->data = data;
> > + cbd->ldd = ldd;
>
> You can't really do that. There's no guarantee that the core atom will
> keep this object around for the lifetime of the driver transaction.
That's interesting. Can I pass const struct ofono_lte *lte, or the
same constraints apply?
Or some other object that I can rely upon?
Also, the core atom shouldn't be called until the callback is triggered.
> Why do you need this anyway? Is it just to build the +CGAUTH part? Why
> don't you l_strdup_printf the +CGAUTH commmand or something instead and
> invoke it in the callback?
>
> E.g. cbd = cb_data_new();
> char *cgauth_cmd = g_strdup_printf("CGAUTH=0,1,"%s","%s", username,
> password);
I will check this, also for compatibility with the switch(vendor) to
come, but I have to confess
I don't really like building the command somewhere and using it later.
It is unexpected for someone reading the code.
What if I have to chain more commands?
>
> cbd->user = cgauth_cmd;
>
> Alternatively you can queue both commands and save the +CGAUTH command
> id inside ldd. Then if +CGDCONT fails, you can g_at_chat_cancel the
> +CGAUTH.
this possibility is also interesting, but equally confusing: I pipe
two commands, and later remove one,
if someone follows the code up to this function, the removal might go unnoticed.
>
> > + cbd->info = info;
> > +
> > + switch (info->proto) {
> > + case OFONO_GPRS_PROTO_IPV6:
> > + proto = "IPV6";
> > + break;
> > + case OFONO_GPRS_PROTO_IPV4V6:
> > + proto = "IPV4V6";
> > + break;
> > + case OFONO_GPRS_PROTO_IP:
> > + proto = "IP";
> > + break;
> > + }
> > +
> > + len = snprintf(buf, sizeof(buf), "AT+CGDCONT=0,\"%s\"", proto);
> > +
> > + if (*info->apn)
> > + snprintf(buf+len, sizeof(buf)-len, ",\"%s\"", info->apn);
> >
> > - /* We can't do much in case of failure so don't check response. */
> > if (g_at_chat_send(ldd->chat, buf, NULL,
> > - at_lte_set_default_attach_info_cb, cbd, g_free) > 0)
> > + at_lte_set_default_attach_info_cb, cbd, NULL) > 0)
>
> Why are you removing the destructor. This will cause leaks whenever a
> hot-unplug event happens and this command is queued...
>
> If you want to use cbd across multiple commands, the proper way to do
> that is with a reference counted structure.
is there an example elsewhere in the code?
>
> > return;
> >
> > + g_free(cbd);
> > CALLBACK_WITH_FAILURE(cb, data);
> > }
> >
> >
>
> Regards,
> -Denis
Regards,
Giacinto
_______________________________________________
ofono mailing list
[email protected]
https://lists.ofono.org/mailman/listinfo/ofono
