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

[Emil Velikov]

On 7 June 2017 at 09:40, Michel Dänzer <michel at daenzer.net> wrote:
> 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.
>
A simple "line = NULL" will lead to a memory leak, AFAICT.

In either case, I'm a bit baffled how that is affected by the
presence/lack of strdup?
We don't alter or reuse the backing storage only
strcmp/isblank/strtol/strdup it.

>
>>> +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.
>
Wouldn't be better to do that in parse_one_line() around the
marketing_name = strdup(...) call?
It's a matter of taste, so feel free to ignore me.

>
>>> +       /* 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.
>
D'oh, indeed. Thank you. Worth adding a note?

>
>>> +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.
>
it's not obvious and might come to bite. Since *p_asic_id_table is
already NULL (we're using calloc) I'd opt for dropping it.
Not trying to force my opinion, just stating concerns.

Another crazy idea that just came to mind:
Since getline() can do multiple implicit realloc's one can allocate "a
sane" default and feed that instead of the current NULL.

Regards,
Emil