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
