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
