* [PATCH v3] tty: vt: keyboard: add default switch-case, to handle smatch-warnings in method vt_do_kdgkb_ioctl
@ 2021-11-06 22:03 Ajay Garg
2021-11-06 22:12 ` Pavel Skripkin
0 siblings, 1 reply; 5+ messages in thread
From: Ajay Garg @ 2021-11-06 22:03 UTC (permalink / raw)
To: gregkh, jirislaby, andriy.shevchenko, kernel, linux-serial, linux-kernel
Cc: paskripkin, Ajay Garg
smatch-kchecker gives the following warnings when run on keyboard.c :
vt_do_kdgkb_ioctl() error: uninitialized symbol 'kbs'.
vt_do_kdgkb_ioctl() error: uninitialized symbol 'ret'.
This usually happens when switch has no default case and static
analyzers and even sometimes compilers can’t prove that all possible
values are covered.
Thus, the default switch-case has been added, which sets the values
for the two variables :
* kbs as NULL, which also nicely fits in with kfree.
* ret as -ENOIOCTLCMD (on same lines if there is no cmd
match in "vt_do_kdskled" method).
Many thanks to the following for review of previous versions :
* Pavel Skripkin <paskripkin@gmail.com>
* Andy Shevchenko <andy.shevchenko@gmail.com>
Signed-off-by: Ajay Garg <ajaygargnsit@gmail.com>
---
There were discussions previously, and the current patch is the
result.
v1 :
https://lore.kernel.org/linux-serial/YYZN30qfaKMskVwE@kroah.com/T/#t
v2 :
https://lore.kernel.org/linux-serial/CAHP4M8Vdj4Eb8q773BeHvsW9n6t=3n1WznuXAR4fZCNi1J6rOg@mail.gmail.com/T/#m18f45676feaba6b1f01ddd5fe607997b190ef4b9
Changes in v2 :
* Changes as required by scripts/checkpatch.pl
* Checking whether kbs is not NULL before kfree is not required,
as kfree(NULL) is safe. So, dropped the check.
Changes in v3 :
* Using default-switch case, and setting the variables
when there is no matching cmd.
drivers/tty/vt/keyboard.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/drivers/tty/vt/keyboard.c b/drivers/tty/vt/keyboard.c
index c7fbbcdcc346..b83e7669658d 100644
--- a/drivers/tty/vt/keyboard.c
+++ b/drivers/tty/vt/keyboard.c
@@ -2090,6 +2090,12 @@ int vt_do_kdgkb_ioctl(int cmd, struct kbsentry __user *user_kdgkb, int perm)
ret = 0;
break;
+ default: {
+ kbs = NULL;
+ ret = -ENOIOCTLCMD;
+
+ break;
+ }
}
kfree(kbs);
--
2.30.2
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v3] tty: vt: keyboard: add default switch-case, to handle smatch-warnings in method vt_do_kdgkb_ioctl
2021-11-06 22:03 [PATCH v3] tty: vt: keyboard: add default switch-case, to handle smatch-warnings in method vt_do_kdgkb_ioctl Ajay Garg
@ 2021-11-06 22:12 ` Pavel Skripkin
2021-11-06 22:17 ` Ajay Garg
0 siblings, 1 reply; 5+ messages in thread
From: Pavel Skripkin @ 2021-11-06 22:12 UTC (permalink / raw)
To: Ajay Garg, gregkh, jirislaby, andriy.shevchenko, kernel,
linux-serial, linux-kernel
On 11/7/21 01:03, Ajay Garg wrote:
> diff --git a/drivers/tty/vt/keyboard.c b/drivers/tty/vt/keyboard.c
> index c7fbbcdcc346..b83e7669658d 100644
> --- a/drivers/tty/vt/keyboard.c
> +++ b/drivers/tty/vt/keyboard.c
> @@ -2090,6 +2090,12 @@ int vt_do_kdgkb_ioctl(int cmd, struct kbsentry __user *user_kdgkb, int perm)
>
> ret = 0;
> break;
> + default: {
> + kbs = NULL;
> + ret = -ENOIOCTLCMD;
> +
> + break;
> + }
Are these brackets needed here? There are no local variables inside
default case.
just my 2c,
With regards,
Pavel Skripkin
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v3] tty: vt: keyboard: add default switch-case, to handle smatch-warnings in method vt_do_kdgkb_ioctl
2021-11-06 22:12 ` Pavel Skripkin
@ 2021-11-06 22:17 ` Ajay Garg
2021-11-06 22:25 ` Pavel Skripkin
0 siblings, 1 reply; 5+ messages in thread
From: Ajay Garg @ 2021-11-06 22:17 UTC (permalink / raw)
To: Pavel Skripkin
Cc: gregkh, jirislaby, andriy.shevchenko, kernel, linux-serial, linux-kernel
On Sun, Nov 7, 2021 at 3:42 AM Pavel Skripkin <paskripkin@gmail.com> wrote:
>
> On 11/7/21 01:03, Ajay Garg wrote:
> > diff --git a/drivers/tty/vt/keyboard.c b/drivers/tty/vt/keyboard.c
> > index c7fbbcdcc346..b83e7669658d 100644
> > --- a/drivers/tty/vt/keyboard.c
> > +++ b/drivers/tty/vt/keyboard.c
> > @@ -2090,6 +2090,12 @@ int vt_do_kdgkb_ioctl(int cmd, struct kbsentry __user *user_kdgkb, int perm)
> >
> > ret = 0;
> > break;
> > + default: {
> > + kbs = NULL;
> > + ret = -ENOIOCTLCMD;
> > +
> > + break;
> > + }
>
> Are these brackets needed here? There are no local variables inside
> default case.
Hmm,
* case KDGKBSENT: uses braces.
* case KDSKBSENT: does not use braces.
I based the layout for default-case on the same lines as case
KDGKBSENT: , as I prefer explicit braces :)
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v3] tty: vt: keyboard: add default switch-case, to handle smatch-warnings in method vt_do_kdgkb_ioctl
2021-11-06 22:17 ` Ajay Garg
@ 2021-11-06 22:25 ` Pavel Skripkin
2021-11-07 3:21 ` Ajay Garg
0 siblings, 1 reply; 5+ messages in thread
From: Pavel Skripkin @ 2021-11-06 22:25 UTC (permalink / raw)
To: Ajay Garg
Cc: gregkh, jirislaby, andriy.shevchenko, kernel, linux-serial, linux-kernel
On 11/7/21 01:17, Ajay Garg wrote:
> On Sun, Nov 7, 2021 at 3:42 AM Pavel Skripkin <paskripkin@gmail.com> wrote:
>>
>> On 11/7/21 01:03, Ajay Garg wrote:
>> > diff --git a/drivers/tty/vt/keyboard.c b/drivers/tty/vt/keyboard.c
>> > index c7fbbcdcc346..b83e7669658d 100644
>> > --- a/drivers/tty/vt/keyboard.c
>> > +++ b/drivers/tty/vt/keyboard.c
>> > @@ -2090,6 +2090,12 @@ int vt_do_kdgkb_ioctl(int cmd, struct kbsentry __user *user_kdgkb, int perm)
>> >
>> > ret = 0;
>> > break;
>> > + default: {
>> > + kbs = NULL;
>> > + ret = -ENOIOCTLCMD;
>> > +
>> > + break;
>> > + }
>>
>> Are these brackets needed here? There are no local variables inside
>> default case.
>
> Hmm,
>
> * case KDGKBSENT: uses braces.
> * case KDSKBSENT: does not use braces.
>
> I based the layout for default-case on the same lines as case
> KDGKBSENT: , as I prefer explicit braces :)
>
I am not against these braces, but I, honestly, dislike them, because
`case : {` syntax looks ugly _to me_.
KDGKBSENT uses it because of local variable `len` and not using them
will cause build error.
I didn't find any strict requirements to not use brackets when there is
no local variable, so it's up to maintainers (again).
Anyway, thank for respinning :)
With regards,
Pavel Skripkin
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v3] tty: vt: keyboard: add default switch-case, to handle smatch-warnings in method vt_do_kdgkb_ioctl
2021-11-06 22:25 ` Pavel Skripkin
@ 2021-11-07 3:21 ` Ajay Garg
0 siblings, 0 replies; 5+ messages in thread
From: Ajay Garg @ 2021-11-07 3:21 UTC (permalink / raw)
To: Pavel Skripkin
Cc: gregkh, jirislaby, andriy.shevchenko, kernel, linux-serial, linux-kernel
> >
>
> I am not against these braces, but I, honestly, dislike them, because
> `case : {` syntax looks ugly _to me_.
>
> KDGKBSENT uses it because of local variable `len` and not using them
> will cause build error.
Ahh, that explains the difference.
Floated the v4 patch at :
https://lore.kernel.org/linux-serial/20211107031721.4734-1-ajaygargnsit@gmail.com/T/#u
so that consistency is maintained in the styling.
Let's continue on the v4-patch link now.
Thanks again Pavel.
>
> I didn't find any strict requirements to not use brackets when there is
> no local variable, so it's up to maintainers (again).
>
>
> Anyway, thank for respinning :)
>
>
>
> With regards,
> Pavel Skripkin
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-11-07 3:25 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-11-06 22:03 [PATCH v3] tty: vt: keyboard: add default switch-case, to handle smatch-warnings in method vt_do_kdgkb_ioctl Ajay Garg
2021-11-06 22:12 ` Pavel Skripkin
2021-11-06 22:17 ` Ajay Garg
2021-11-06 22:25 ` Pavel Skripkin
2021-11-07 3:21 ` Ajay Garg
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).