[PATCH] qmi-codegen: don't mark missing optional TLVs as set

[Aleksander Morgado]

On Fri, Nov 21, 2014 at 8:33 PM, Dan Williams <dcbw at redhat.com> wrote:
> In the optional TLV case, 'tlv_error' was never set to TRUE
> when the optional TLV was missing, because the return value of
> qmi_message_tlv_read_init() was ignored.  This caused
> "self->arg_*_set" to always be TRUE and NULL strings to be
> returned to callers requesting the TLV value later.
>
> Also prevent incorrect "Left X bytes unread when getting..."
> messages caused when the TLV is missing.
>
> This bug was found when probing a modem that does not return
> an MEID TLV to DMSGetIds (because it is GSM/UMTS/LTE only),
> but qmi_message_dms_get_ids_output_get_meid() returned TRUE
> and a NULL 'str':
>
>     if (qmi_message_dms_get_ids_output_get_meid (output, &str, NULL) &&
> -->     str[0] != '\0' && str[0] != '0') {
>
> Bug introduced in b143b7f6 (qmi-codegen: use the new TLV reader API).
>
> Before:
> =======
>
>     gsize offset = 0;
>     gsize init_offset;
>     gboolean tlv_error = FALSE;
>
>     init_offset = qmi_message_tlv_read_init (message, QMI_INDICATION_DMS_EVENT_REPORT_OUTPUT_TLV_POWER_STATE, NULL, NULL);
>     <<<snip>>>
>
>     /* The remaining size of the buffer needs to be 0 if we successfully read the TLV */
>     if ((offset = __qmi_message_tlv_read_remaining_size (message, init_offset, offset)) > 0) {
>         g_warning ("Left '%" G_GSIZE_FORMAT "' bytes unread when getting the 'Power State' TLV", offset);
>     }
>
> qmi_indication_dms_event_report_output_power_state_out:
>     if (!tlv_error)
>         self->arg_power_state_set = TRUE;
>
> After:
> ======
>
>     gsize offset = 0;
>     gsize init_offset;
>
>     if ((init_offset = qmi_message_tlv_read_init (message, QMI_INDICATION_DMS_EVENT_REPORT_OUTPUT_TLV_POWER_STATE, NULL, NULL)) == 0) {
>         goto qmi_indication_dms_event_report_output_power_state_out;
>     }
>     <<<snip>>>
>
>     /* The remaining size of the buffer needs to be 0 if we successfully read the TLV */
>     if ((offset = __qmi_message_tlv_read_remaining_size (message, init_offset, offset)) > 0) {
>         g_warning ("Left '%" G_GSIZE_FORMAT "' bytes unread when getting the 'Power State' TLV", offset);
>     }
>
>     self->arg_power_state_set = TRUE;
>
> qmi_indication_dms_event_report_output_power_state_out:
>     ;
>

Nice catch! Go on and merge, please.

> ---
>  build-aux/qmi-codegen/Field.py | 27 ++++++++++++++++-----------
>  1 file changed, 16 insertions(+), 11 deletions(-)
>
> diff --git a/build-aux/qmi-codegen/Field.py b/build-aux/qmi-codegen/Field.py
> index 7d51b93..c64bf29 100644
> --- a/build-aux/qmi-codegen/Field.py
> +++ b/build-aux/qmi-codegen/Field.py
> @@ -280,23 +280,25 @@ class Field:
>          template = (
>              '${lp}gsize offset = 0;\n'
>              '${lp}gsize init_offset;\n'
> -            '${lp}gboolean tlv_error = FALSE;\n'
>              '\n'
> -            '${lp}init_offset = qmi_message_tlv_read_init (message, ${tlv_id}, NULL, ${error});\n')
> +            '${lp}if ((init_offset = qmi_message_tlv_read_init (message, ${tlv_id}, NULL, ${error})) == 0) {\n')
> +
>          if self.mandatory:
>              template += (
> -                '\n'
> -                '${lp}if (init_offset == 0) {\n'
>                  '${lp}    g_prefix_error (${error}, "Couldn\'t get the mandatory ${name} TLV: ");\n'
>                  '${lp}    ${container_underscore}_unref (self);\n'
> -                '${lp}    return NULL;\n'
> -                '${lp}}\n')
> +                '${lp}    return NULL;\n')
> +        else:
> +            template += (
> +                '${lp}    goto ${tlv_out};\n')
>
> +        template += (
> +            '${lp}}\n')
>
>          f.write(string.Template(template).substitute(translations))
>
>          # Now, read the contents of the buffer into the variable
> -        self.variable.emit_buffer_read(f, line_prefix + '    ', tlv_out, error, 'self->' + self.variable_name)
> +        self.variable.emit_buffer_read(f, line_prefix, tlv_out, error, 'self->' + self.variable_name)
>
>          template = (
>              '\n'
> @@ -305,15 +307,18 @@ class Field:
>              '${lp}    g_warning ("Left \'%" G_GSIZE_FORMAT "\' bytes unread when getting the \'${name}\' TLV", offset);\n'
>              '${lp}}\n'
>              '\n'
> -            '${tlv_out}:\n'
> -            '${lp}if (!tlv_error)\n'
> -            '${lp}    self->${variable_name}_set = TRUE;\n')
> +            '${lp}self->${variable_name}_set = TRUE;\n'
> +            '\n'
> +            '${tlv_out}:\n')
>          if self.mandatory:
>              template += (
> -                '${lp}else {\n'
> +                '${lp}if (!self->${variable_name}_set) {\n'
>                  '${lp}    ${container_underscore}_unref (self);\n'
>                  '${lp}    return NULL;\n'
>                  '${lp}}\n')
> +        else:
> +            template += (
> +                '${lp};\n')
>          f.write(string.Template(template).substitute(translations))
>
>
> --
> 1.9.3
>
>



-- 
Aleksander
https://aleksander.es