Hi Samuel,
With all the other discussion aside here is some code specific input
which I'd hope you agree with.
On 31 May 2017 at 21:22, Samuel Li <Samuel.Li at amd.com> wrote:
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -45,6 +45,9 @@ AM_DISTCHECK_CONFIGURE_FLAGS = \
>
> pkgconfigdir = @pkgconfigdir@
> pkgconfig_DATA = libdrm.pc
> +libdrmdatadir = $(datadir)/libdrm
> +dist_libdrmdata_DATA = include/drm/amdgpu.ids
> +export libdrmdatadir
Don't place exports in the makefiles. See how pkgconfigdir is managed
in configure.ac and do do with libdrmdatadir.
> --- /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.
> + if (!buf)
> + return -ENOMEM;
> +
> + /* ignore empty line and commented line */
> + if (strlen(line) == 0 || line[0] == '#') {
You might want to check/trim whitespace before #? Same question applies below.
> + r = -EAGAIN;
> + goto out;
With the strdup, hence free() gone, all the errors will be "return -EFOO;"
> + /* trim leading whitespaces or tabs */
> + while (*s_name == ' ' || *s_name == '\t')
> + s_name++;
Use isblank/isspace - they should handle \r et al, which will creep as
someone on the team uses Windows ;-)
> +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?
Same question applies for the second loop, below.
> +
> + /* ignore empty line and commented line */
> + if (strlen(line) == 0 || line[0] == '#') {
> + line_num++;
> + continue;
> + }
> +
> + drmMsg("%s version: %s\n", AMDGPU_ASIC_ID_TABLE, line);
> + break;
> + }
> +
Should the lack of a version line/tag be considered fatal or not? An
inline comment is welcome.
> + while ((n = getline(&line, &len, fp)) != -1) {
> +
> + if (table_size >= table_max_size) {
Move it at the top of the loop, since as-is it will end up reallocate
even when it doesn't need to.
> + /* double table size */
> + table_max_size *= 2;
> + asic_id_table = realloc(asic_id_table, table_max_size *
> + sizeof(struct amdgpu_asic_id));
> + if (!asic_id_table) {
Memory originally pointed by asic_id_table is leaked.
> + r = -ENOMEM;
> + goto free;
> + }
> + }
> + }
> +
> + /* 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?
> + if (!asic_id_table)
As above - we have a leak original asic_id_table.
> + r = -ENOMEM;
> +
> +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.
Regards,
Emil
