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

You can if you want.

>
> Raag

-- 
Jani Nikula, Intel