qmicli: NAS set technology preference

[Aleksander Morgado]

Hey André,

Patch review below.


On 15/01/13 16:30, A. Valentin wrote:
> diff --git a/cli/qmicli-nas.c b/cli/qmicli-nas.c

Please next time give me a patch generated with format-patch, so that I
get your author info and commit message.


> index 751ce3f..55c3f11 100644
> --- a/cli/qmicli-nas.c
> +++ b/cli/qmicli-nas.c
> @@ -44,6 +44,7 @@ static Context *ctx;
>  static gboolean get_signal_strength_flag;
>  static gboolean get_signal_info_flag;
>  static gboolean get_home_network_flag;
> +static gchar *set_technology_preference_str;
>  static gboolean get_serving_system_flag;
>  static gboolean get_system_info_flag;
>  static gboolean get_technology_preference_flag;
> @@ -77,6 +78,10 @@ static GOptionEntry entries[] = {
>        "Get technology preference",
>        NULL
>      },
> +    { "nas-set-technology-preference", 0, 0, G_OPTION_ARG_STRING, &set_technology_preference_str,
> +      "Set technolovy preference",
> +      "ALL|EVDO|AMPS|GSM|CDMA|UMTS|LTE",

Your implementation, what does accept? A list of the previous items?
Just one?


> +    },
>      { "nas-get-system-selection-preference", 0, 0, G_OPTION_ARG_NONE, &get_system_selection_preference_flag,
>        "Get system selection preference",
>        NULL
> @@ -123,6 +128,7 @@ qmicli_nas_options_enabled (void)
>      n_actions = (get_signal_strength_flag +
>                   get_signal_info_flag +
>                   get_home_network_flag +
> +                 !!set_technology_preference_str +
>                   get_serving_system_flag +
>                   get_system_info_flag +
>                   get_technology_preference_flag +
> @@ -458,6 +464,24 @@ get_signal_strength_ready (QmiClientNas *client,
>  }
>  
>  static void
> +set_technology_preference_ready (QmiClientNas *client,
> +                        GAsyncResult *res)

Alignment fix needed here.

> +{
> +    QmiMessageNasSetTechnologyPreferenceOutput *output;
> +    GError *error = NULL;
> +
> +    output = qmi_client_nas_set_technology_preference_finish(client, res, &error);


Please add a whitespace before the '(' always; lots of places in the
patch to fix this, almost in every new line :)


> +    if (!output) {
> +        g_printerr ("error: operation failed: %s\n", error->message);
> +        g_error_free (error);
> +        shutdown (FALSE);
> +        return;
> +    }
> +    qmi_message_nas_set_technology_preference_output_unref (output);
> +    shutdown (TRUE);
> +}
> +
> +static void
>  get_home_network_ready (QmiClientNas *client,
>                          GAsyncResult *res)
>  {
> @@ -2051,6 +2075,50 @@ qmicli_nas_run (QmiDevice *device,
>          return;
>      }
>  
> +    /* Set technology preference */
> +    if (set_technology_preference_str) {
> +        QmiMessageNasSetTechnologyPreferenceInput *input = NULL;
> +        QmiNasPreferenceDuration duration = QMI_NAS_PREFERENCE_DURATION_PERMANENT;
> +        int modepreference = QMI_NAS_RADIO_TECHNOLOGY_PREFERENCE_AUTO;

'gint' better here for consistency; not a big deal anyway.

> +
> +        if (set_technology_preference_str[0] &&  strlen(set_technology_preference_str)) {

Checking str[0] is actually equivalent to using strlen(str) here. You
just want to know if the string is not empty, both give you that
information. Using only str[0] is just better (i.e. there is at least
one character), as you don't really need to get the whole string length.


> +            gint n = 0;
> +            char ** fields = g_strsplit (set_technology_preference_str, ",", 0);

Don't call the method directly when declaring the variable here. First
declare the variable, then whitespace, then the g_strsplit().

> +
> +            for(n=0; n<g_strv_length(fields); n++ ) {

Add whitespaces also before and after '=', '<'...


> +                if (fields[n] == NULL || strlen(fields[n]) == 0) {

fields[n] == NULL will never really happen.

> +                    continue;
> +                }
> +                if (g_ascii_strcasecmp(fields[n], "EVDO") == 0) {
> +                    modepreference = modepreference | QMI_NAS_RADIO_TECHNOLOGY_PREFERENCE_3GPP | QMI_NAS_RADIO_TECHNOLOGY_PREFERENCE_3GPP2 | QMI_NAS_RADIO_TECHNOLOGY_PREFERENCE_HDR;

EVDO is 3GPP2 only, no need to add 3GPP here.

> +                } else if (g_ascii_strcasecmp(fields[n], "AMPS") == 0) {
> +                    modepreference = modepreference | QMI_NAS_RADIO_TECHNOLOGY_PREFERENCE_3GPP2 | QMI_NAS_RADIO_TECHNOLOGY_PREFERENCE_AMPS_OR_GSM;
> +                } else if (g_ascii_strcasecmp(fields[n], "GSM") == 0) {
> +                    modepreference = modepreference | QMI_NAS_RADIO_TECHNOLOGY_PREFERENCE_3GPP | QMI_NAS_RADIO_TECHNOLOGY_PREFERENCE_AMPS_OR_GSM;
> +                } else if (g_ascii_strcasecmp(fields[n], "CDMA") == 0) {
> +                    modepreference = modepreference | QMI_NAS_RADIO_TECHNOLOGY_PREFERENCE_3GPP2 | QMI_NAS_RADIO_TECHNOLOGY_PREFERENCE_CDMA_OR_WCDMA;
> +                } else if (g_ascii_strcasecmp(fields[n], "UMTS") == 0) {
> +                    modepreference = modepreference | QMI_NAS_RADIO_TECHNOLOGY_PREFERENCE_3GPP | QMI_NAS_RADIO_TECHNOLOGY_PREFERENCE_CDMA_OR_WCDMA;
> +                } else if (g_ascii_strcasecmp(fields[n], "LTE") == 0) {
> +                    modepreference = modepreference | QMI_NAS_RADIO_TECHNOLOGY_PREFERENCE_3GPP | QMI_NAS_RADIO_TECHNOLOGY_PREFERENCE_3GPP2 | QMI_NAS_RADIO_TECHNOLOGY_PREFERENCE_LTE;

In this very specific case with LTE, I believe that adding the 3GPP or
3GPP2 flag is not needed. Worth checking that.


> +                }
> +            }
> +            g_strfreev(fields);
> +            fields = NULL;
> +        }

There is a problem with this implementation. It seems you allow
specifying 3GPP and 3GPP2 technologies at the same time, e.g. "CDMA" and
"UMTS" at the same time, but that is not ok. Either you specify one or
more 3GPP technologies or you specify one or more 3GPP2 technologies,
never both at the same time. The exception is LTE AFAIK, which should be
allowed in both sets.

So, I would add some more checks on the string contents, so that if a
3GPP tech is given (GSM or UMTS), we don't allow a 3GPP2 tech
(AMPS,CDMA,EVDO), and vice-versa.


> +
> +        input = qmi_message_nas_set_technology_preference_input_new();
> +        qmi_message_nas_set_technology_preference_input_set_current(input, modepreference, duration, NULL);
> +        qmi_client_nas_set_technology_preference(ctx->client,
> +                                         input,

Multiple alignment fixes here needed.

> +                                         10,
> +                                         ctx->cancellable,
> +                                         (GAsyncReadyCallback)set_technology_preference_ready,
> +                                         NULL);
> +        qmi_message_nas_set_technology_preference_input_unref(input);
> +        return;
> +    }
> +
>      /* Request to get serving system? */
>      if (get_serving_system_flag) {
>          g_debug ("Asynchronously getting serving system...");


-- 
Aleksander