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

[Raag Jadav <raag.jadav@intel.com>]

On Thu, Sep 19, 2024 at 12:24:09PM +0300, Jani Nikula wrote:
> 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.

On that note...

> > 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.

...should we use kasprintf instead of hardcoding size?

Raag