[PATCH libdrm v6 1/1] amdgpu: move asic id table to a separate file

[Michel Dänzer]

On 06/06/17 10:43 PM, Emil Velikov wrote:
> On 31 May 2017 at 21:22, Samuel Li <Samuel.Li at amd.com> wrote:
> 
>> --- /dev/null
>> +++ b/amdgpu/amdgpu_asic_id.c
> 
>> +static int parse_one_line(const char *line, struct amdgpu_asic_id *id)
>> +{
>> +       char *buf, *saveptr;
>> +       char *s_did;
>> +       char *s_rid;
>> +       char *s_name;
>> +       char *endptr;
>> +       int r = 0;
>> +
>> +       buf = strdup(line);
> You don't need the extra strdup here if you use strchr over strtok.

Beware that without strdup here, amdgpu_parse_asic_ids must set line =
NULL after table_line++, so that getline allocates a new buffer for the
next line.


>> +int amdgpu_parse_asic_ids(struct amdgpu_asic_id **p_asic_id_table)
>> +{
> 
>> +       /* 1st valid line is file version */
>> +       while ((n = getline(&line, &len, fp)) != -1) {
>> +               /* trim trailing newline */
>> +               if (line[n - 1] == '\n')
>> +                       line[n - 1] = '\0';
> Why do we need this - afaict none of the parsing code cares if we have
> \n or not?

The \n has to be removed somehow, otherwise it ends up as part of the
marketing name returned to the application.


>> +       /* end of table */
>> +       id = asic_id_table + table_size;
>> +       memset(id, 0, sizeof(struct amdgpu_asic_id));
> Here one clears the sentinel, which is needed as we hit realloc above, correct?
> 
>> +       asic_id_table = realloc(asic_id_table, (table_size+1) *
>> +                               sizeof(struct amdgpu_asic_id));
> But why do we realloc again?

I asked for that, in order not to waste memory for unused table entries.


>> +free:
>> +       free(line);
>> +
>> +       if (r && asic_id_table) {
>> +               while (table_size--) {
>> +                       id = asic_id_table + table_size;
>> +                       free(id->marketing_name);
>> +               }
>> +               free(asic_id_table);
>> +               asic_id_table = NULL;
>> +       }
>> +close:
>> +       fclose(fp);
>> +
>> +       *p_asic_id_table = asic_id_table;
>> +
> Please don't entwine the error path with the normal one.
> 
> Setting *p_asic_id_table (or any user provided pointer) when the
> function fails is bad design.

I don't really see the issue with that; it's fine for the only caller of
this function.


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer