Re: [PATCH v4 09/10] tools: spkgimage: add Renesas SPKG format

[Ralph Siemsen <ralph.siemsen@linaro.org>]

On Mon, Apr 17, 2023 at 07:23:46PM +0200, Marek Vasut wrote:
>On 3/8/23 21:26, Ralph Siemsen wrote:
>>+			spkgimage.o \
>
>Maybe just call the file renesas_spkgimage.o so its clear which 
>SoC/vendor this file is associtated with.

Okay, will do.

>>+static struct spkg_file out_buf;
>>+
>>+static uint32_t padding;
>
>Is this padding here and the padding in struct config_file below 
>different padding ? Can we get rid of these static global variables ?

I will give it a try.

>
>>+static int check_range(const char *name, int val, int min, int max)
>>+{
>>+	if (val < min) {
>>+		fprintf(stderr, "Warning: param '%s' adjusted to min %d\n",
>>+			name, min);
>>+		val = min;
>>+	}
>>+
>>+	if (val > max) {
>>+		fprintf(stderr, "Warning: param '%s' adjusted to max %d\n",
>>+			name, max);
>>+		val = max;
>>+	}
>
>There is a macro clamp() which implements range limiting .

Thanks for pointing that out. However I think there is value in the 
diagnostic print when the value is clamped. Ideally it should help the 
user to fix their invoking script/binman/etc.

Of course, I could call clamp() and check if the value differs, but that 
seems just as complex as the check_range().

>>+	while (fgets(line, sizeof(line), fcfg)) {
>>+		line_num += 1;
>>+
>>+		/* Skip blank lines and comments */
>>+		if (line[0] == '\n' || line[0] == '#')
>>+			continue;
>>+
>>+		/* Strip the trailing newline */
>>+		len = strlen(line);
>>+		if (line[len - 1] == '\n')
>>+			line[--len] = 0;
>
>Use len - 1 here too to avoid confusion ?

Old habit. I always try to update the length in sync with modifying the 
string. If done as a separate line/statement, it is more likely to be 
lost during subsequent modifications.

In this case I do not need "len" at all, so I could just do:

		line[strcspn(line, "\n")] = 0;

Ralph