[Patch] add qmicli support to create/modify/delete PDP profiles

[Wolfgang Tolkien]

See answers below...

‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
On August 5, 2018 2:47 AM, Aleksander Morgado <aleksander at aleksander.es> wrote:

> Hey,
>
> On Thu, Jul 19, 2018 at 8:34 PM, Wolfgang Tolkien w at tolkien.email wrote:
>
> > This adds qmicli support to create/modify/delete PDP profiles.
> >
> > -   This patch only supports setting APN/auth/etc. using the 'modify'
> >     command. Creating a new profile is therefore a 2-step process: create and
> >     then modify.
> >
> > -   Syntax is inconsistent: create/delete use old syntax since there are only
> >     one or two parameters, whereas the modify command uses key/value syntax.
> >     This mirrors other existing commands, but I'm happy to change it if people
> >     are unhappy with it.
> >
>
> But "create" allows all parameters that are available in "modify",
> right? I think we should add those as well. Would you be up to doing
> that?
OK, I should be able to do that in the next couple of days.

>
> I'm thinking that we could have --wds-create-profile allow key/value
> pairs as well, but also predefined fields at the start of the string,
> e.g. "3gpp" or "3gpp2". This could allow us to have for example
> actions like this:
> --wds-create-profile="3gpp,apn=internet,pdp-type=ipv4"
>
> Then, --wds-swi-create-profile would allow the same list of key/value
> pairs, but also requiring 2 predefined fields at the start of the
> string, "3gpp" or "3gpp2" plus the profile index, e.g.:
> --wds-swi-create-profile="3gpp,4,apn=internet,pdp-type=ipv4"
>
> So --wds-modify-profile would follow the same logic of 2 predefined
> fields with the exact same format as --wds-swi-create-profile, e.g.:
> --wds-modify-profile="3gpp,4,apn=internet,pdp-type=ipv4"
>
> And the same for --wds-delete-profile:
> --wds-modify-profile="3gpp,4"
>
> This would remove also the inconsistency between "3gpp-profile=#" and "3gpp,#"

OK, good idea. I will use this format.

>
> > -   Modems seem to allow empty strings for APN/Name/Username/Password (which
> >     makes sense if one wants to delete the value entirely), however libqmi does
> >     not allow this. This patch does not address this. Suggested workaround is to
> >     delete the profile and recreate it.
> >
>
> Is this a limitation of the code generator? If so, maybe we should
> have a "allow-empty" : "yes" property for this kind of cases?

It is a limitation of the code generator. I haven't looked at that part of libqmi yet, so I would prefer if that could be addressed with a separate patchset, possibly authored by someone else...


>
> Other comments:
>
> I see when preparing the SWI Create Profile Indexed command that
> you're setting the PDP context number to be equal to the WDS Profile
> Index:
>
> -         qmi_message_wds_swi_create_profile_indexed_input_set_profile_identifier
>
>
>
> (input, profile_type, (guint8)profile_index, NULL);
>
> -         qmi_message_wds_swi_create_profile_indexed_input_set_pdp_context_number
>
>
>
> (input, (guint8)profile_index, NULL);
>
> Shouldn't we allow specifying a different PDP context number
> associated to the WDS profile index?
>
> In, qmicli_read_pdp_type_from_string() I see:
>
> -   else if (!str[0] || g_ascii_strcasecmp (str, "BOTH") == 0 ||
>     g_ascii_strcasecmp (str, "IPV4V6") == 0)
>
> -          *out = QMI_WDS_PDP_TYPE_IPV4_OR_IPV6;
>
>
>
> I don't think we should match the empty string or "both" to IPv4v6.
> The user should set IPv4v6.


OK, I'll change that. I picked those defaults to match the behaviour of the AT+CGDCONT command, but giving the user more flexibility also OK with me.

For reference, AT+CGDCONT=[...] will set both PDP context number and WDS Profile Index to the same value. The AT+CGDCONT? command displays the PDP context number. If that's not the same as the WDS Profile Index, then there is no way to determine the WDS profile index using AT commands.


Thanks,
Wolfgang