Re: [PATCH v4] tty: vt: keyboard: add default switch-case, to handle smatch-warnings in method vt_do_kdgkb_ioctl

From: Ajay Garg <ajaygargnsit@gmail.com>
To: Johan Hovold <johan@kernel.org>
Cc: gregkh@linuxfoundation.org, jirislaby@kernel.org,
	andriy.shevchenko@linux.intel.com, kernel@esmil.dk,
	linux-serial@vger.kernel.org, linux-kernel@vger.kernel.org,
	paskripkin@gmail.com
Subject: Re: [PATCH v4] tty: vt: keyboard: add default switch-case, to handle smatch-warnings in method vt_do_kdgkb_ioctl
Date: Mon, 8 Nov 2021 19:21:50 +0530	[thread overview]
Message-ID: <CAHP4M8Va6BLY+RYPzZi=_o1M3jcxbywSHJ3FsXZkeDU2T3BphA@mail.gmail.com> (raw)
In-Reply-To: <YYjw2mRIhy1SoIb+@hovoldconsulting.com>

Hi Johan,

Thanks for your time.

On Mon, Nov 8, 2021 at 3:11 PM Johan Hovold <johan@kernel.org> wrote:
>
> First, please fix your patch Subject which is way too verbose. You
> should aim at less than 72 chars including prefix. Something like
>
>         "vt: keyboard: suppress smatch warning in vt_do_kdgkb_ioctl"
>
> should do.

Thanks Johan, will keep this in mind in all my future patches.

Taking the subject as suggested by you for the current patch, after
removal of "smatch" keyword, as it otherwise produces :
WARNING: A patch subject line should describe the change not the tool
that found it
when run through checkpatch.pl

> >       kfree(kbs);
>
> Instead, move the kfree() into the two cases blocks

So, if I understand correctly,

* prior 07edff926520, the scope of kbs (allocation/deallocation) was
external to switch-cases.
* post 07edff926520, kbs is allocated internally for each case,
however the deallocation remains external.

Thanks for the much cleaner suggestion, will move kfree() into the two
cases blocks.

> and initialise ret to 0

Got it, other functions (except vt_do_kdskled) initialize ret to 0.
Thanks again.

> as is done in several other vt helpers in case a driver bug ever
> causes them to be called for the wrong cmds (e.g. instead of sprinkling
> WARN_ON(1) in all those functions).
>
> You may want to mention that the kfree warning was introduced by
>
>         07edff926520 ("vt: keyboard, reorder user buffer handling in vt_do_kdgkb_ioctl")
>
> which moved the shared allocation into the switch statement, and perhaps
> also mention
>
>         4e1404a5cd04 ("vt: keyboard, extract and simplify vt_kdskbsent")
>
> for the ret warning.

Thanks a ton Johan for your time in investigating the history.
This has made things, in the current scenario, crystal clear.

Have floated the v5 patch at :
https://lore.kernel.org/linux-serial/20211108134901.7449-1-ajaygargnsit@gmail.com/T/#u

Let's continue further discussion there.

Thanks and Regards,
Ajay