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

From: Emil Velikov <emil.l.velikov-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
To: Samuel Li <Samuel.Li-5C7GfCeVMHo@public.gmane.org>
Cc: amd-gfx mailing list
	<amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org>,
	Xiaojie Yuan <Xiaojie.Yuan-5C7GfCeVMHo@public.gmane.org>
Subject: Re: [PATCH libdrm v6 1/1] amdgpu: move asic id table to a separate file
Date: Tue, 6 Jun 2017 14:43:16 +0100	[thread overview]
Message-ID: <CACvgo50KM8dvXENsza1ZKbGc_Ww-sDaEVUemKpzjkbsSu+hruQ@mail.gmail.com> (raw)
In-Reply-To: <1496262170-4222-1-git-send-email-Samuel.Li-5C7GfCeVMHo@public.gmane.org>

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@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
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx