Re: [PATCH v5 2/4] drm: Expose wedge recovery methods

[Jani Nikula <jani.nikula@linux.intel.com>]

On Thu, 19 Sep 2024, Raag Jadav <raag.jadav@intel.com> wrote:
> On Thu, Sep 19, 2024 at 10:38:51AM +0300, Jani Nikula wrote:
>> On Thu, 19 Sep 2024, Raag Jadav <raag.jadav@intel.com> wrote:
>> > On Tue, Sep 17, 2024 at 10:49:07AM +0300, Jani Nikula wrote:
>> >> On Tue, 17 Sep 2024, Raag Jadav <raag.jadav@intel.com> wrote:
>> >> >  
>> >> > +extern const char *const wedge_recovery_opts[];
>> >> 
>> >> Data is not an interface. Please add a function for this.
>> >
>> > For a single user?
>> 
>> Yes.
>> 
>> Well, you kind of have two, and both places need to do bounds checking
>> on indexing the array. You also need to do bounds checking on the string
>> manipulation, you can't just strcat and assume it'll be all right.
>
> Which would be true if we were to receive an unknown string. Here we sorta
> know it offhand so we're not gonna shoot in our foot :D

The thing about long term code maintenance is that "we know" often turns
into "not too obvious" and "probably" somewhere down the line, as
features get added and code gets refactored and moved about.

Here, it only takes a new, longer string, and failure to manually check
that the lengths don't exceed the magic 32 bytes. Just be safe from the
start, and you don't have to worry about it later.

> Anyway, would you prefer strlcat instead?

I think the cleaner option is:

	char event_string[32];

	snprintf(event_string, sizeof(event_string), "WEDGED=%s", wedge_name(method));

which is also what most other code constructing environments for
kobject_uevent_env() do.

BR,
Jani.


-- 
Jani Nikula, Intel