Thrift
  1. Thrift
  2. THRIFT-1266

generated C code for iterating over nested maps is wrong

    Details

      Description

      We are working with Cassandra API in C generated by Thrift and have noticed a bug in the generated code for cassandra_client_send_batch_mutate().

      Full code that Thrift generates for this function is attached, but here is the specification for Cassandra's batch_mutate method:

      /**
        Mutate many columns or super columns for many row keys. See also: Mutation.
      
        mutation_map maps key to column family to a list of Mutation objects to take place at that scope.
      **/
      void batch_mutate(1:required map<binary, map<string, list<Mutation>>> mutation_map,
                        2:required ConsistencyLevel consistency_level=ConsistencyLevel.ONE)
           throws (1:InvalidRequestException ire, 2:UnavailableException ue, 3:TimedOutException te),
      

      If we now look at the generated code, we will notice the following fragment:

      GPtrArray * value;
      g_hash_table_foreach ((GHashTable *)  value, thrift_hash_table_get_keys, &key_list); /* LINE A */
      

      We can see that in line A it uses the variable "value" as GHashTable, even though the GHashTable "value" was shadowed by GPtrArray "value" a line before.

      Similarly, we can see another fragment below that one, where one instance of variable "value" shadows another instance:

      value = (GPtrArray *) g_hash_table_lookup (((GHashTable *)  value), (gpointer) key); /* LINE B */
      

      We have worked around the bug in our particular case by renaming one of the "value" variables to "value2" (see "svn di -c 21176 svn://svn.zabbix.com/branches/dev/ZBXNEXT-844/src/libs/zbxcassa/cassandra.c@21176" for a diff), but it would be nice to fix it in Thrift, too.

        Issue Links

          Activity

          Aleksandrs Saveljevs created issue -
          Aleksandrs Saveljevs made changes -
          Field Original Value New Value
          Attachment batch_mutate.c [ 12490649 ]
          Aleksandrs Saveljevs made changes -
          Description We are working with Cassandra API in C generated by Thrift and have noticed a bug in the generated code for cassandra_client_send_batch_mutate().

          Full code that Thrift generates for this function is attached, but here is the specification for Cassandra's batch_mutate method:

          {code}/**
            Mutate many columns or super columns for many row keys. See also: Mutation.

            mutation_map maps key to column family to a list of Mutation objects to take place at that scope.
          **/
          void batch_mutate(1:required map<binary, map<string, list<Mutation>>> mutation_map,
                            2:required ConsistencyLevel consistency_level=ConsistencyLevel.ONE)
               throws (1:InvalidRequestException ire, 2:UnavailableException ue, 3:TimedOutException te),
          {code}

          If we now look at the generated code, we will notice the following fragment:

          {code}GPtrArray * value;
          g_hash_table_foreach ((GHashTable *) value, thrift_hash_table_get_keys, &key_list); /* LINE A */
          {code}

          We can see that in line A it uses the variable "value" as GHashTable, even though the GHashTable "value" was shadowed by GPtrArray "value" a line before.

          Similarly, we can see another fragment below that one, where one instance of variable "value" shadows another instance:

          {code}
          value = (GPtrArray *) g_hash_table_lookup (((GHashTable *) value), (gpointer) key); /* LINE B */
          {code}

          We have worked around the bug in our particular case by renaming one of the "value" variables to "value2" (see "svn di -c 21176 svn://svn.zabbix.com/branches/dev/ZBXNEXT-844/src/libs/zbxcassa/cassandra.c" for a diff), but it would be nice to fix it in Thrift, too.
          We are working with Cassandra API in C generated by Thrift and have noticed a bug in the generated code for cassandra_client_send_batch_mutate().

          Full code that Thrift generates for this function is attached, but here is the specification for Cassandra's batch_mutate method:

          {code}/**
            Mutate many columns or super columns for many row keys. See also: Mutation.

            mutation_map maps key to column family to a list of Mutation objects to take place at that scope.
          **/
          void batch_mutate(1:required map<binary, map<string, list<Mutation>>> mutation_map,
                            2:required ConsistencyLevel consistency_level=ConsistencyLevel.ONE)
               throws (1:InvalidRequestException ire, 2:UnavailableException ue, 3:TimedOutException te),
          {code}

          If we now look at the generated code, we will notice the following fragment:

          {code}GPtrArray * value;
          g_hash_table_foreach ((GHashTable *) value, thrift_hash_table_get_keys, &key_list); /* LINE A */
          {code}

          We can see that in line A it uses the variable "value" as GHashTable, even though the GHashTable "value" was shadowed by GPtrArray "value" a line before.

          Similarly, we can see another fragment below that one, where one instance of variable "value" shadows another instance:

          {code}
          value = (GPtrArray *) g_hash_table_lookup (((GHashTable *) value), (gpointer) key); /* LINE B */
          {code}

          We have worked around the bug in our particular case by renaming one of the "value" variables to "value2" (see "svn di -c 21176 svn://svn.zabbix.com/branches/dev/ZBXNEXT-844/src/libs/zbxcassa/cassandra.c@21176" for a diff), but it would be nice to fix it in Thrift, too.
          Simon South made changes -
          Assignee Simon South [ simonsouth ]
          Simon South made changes -
          Simon South made changes -
          Link This issue is required by THRIFT-2690 [ THRIFT-2690 ]
          Roger Meier made changes -
          Status Open [ 1 ] Resolved [ 5 ]
          Resolution Fixed [ 1 ]
          Jake Farrell made changes -
          Fix Version/s 0.9.2 [ 12324954 ]
          Jake Farrell made changes -
          Status Resolved [ 5 ] Closed [ 6 ]

            People

            • Assignee:
              Simon South
              Reporter:
              Aleksandrs Saveljevs
            • Votes:
              1 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development