Re: [PATCH v7 03/10] drm/xe/devcoredump: Add ASCII85 dump helper function

[John Harrison <john.c.harrison@intel.com>]

On 9/10/2024 12:33, Julia Filipchuk wrote:
>> +	if (prefix) {
>> +		strscpy(line_buff, prefix, DMESG_MAX_LINE_LEN - MIN_SPACE - 3);
>> +		line_pos = strlen(line_buff);
>> +
>> +		line_buff[line_pos++] = ':';
>> +		line_buff[line_pos++] = ' ';
>> +	}
> Since 2 characters are added to the end of prefix should the computation
> for n be "DMESG_MAX_LINE_LEN - MIN_SPACE - 2"? MIN_SPACE already
> accounts for space of trailing newline and null terminator.
Yeah, I think that is left over from when this was adding three extra 
characters rather than just two.

>
> Also, prefix is only added to the first line. Is that the intended behavior?
Yes. The first line tells you what the dump is. Then you just have a 
continuous dump with no interrupts other than the line breaks which can 
be fed directly into a decoder.

>
>
>> +		strscpy(line_buff + line_pos, ascii85_encode(val, buff),
>> +			DMESG_MAX_LINE_LEN - line_pos);
>> +		line_pos += strlen(line_buff + line_pos);
> Since space for ASCII85_BUFSZ bytes is guaranteed by checks against
> MIN_SPACE output of strscpy can be used to increment line_pos. Although
> I do like this safer style that would work even if the checks failed.
>
>> +#undef MIN_SPACE
> Suggest to also #undef DMESG_MAX_LINE_LEN here.
Yup.

John.

>
>
> Reviewed-by: Julia Filipchuk <julia.filipchuk@intel.com>
>