RE: [PATCH 1/3] treewide: Lift switch variables out of switches

2019-01-25 Thread Edwin Zimmerman
On Wednesday, January 23, 2019 6:04 AM, Kees Cook wrote
> 
> Variables declared in a switch statement before any case statements
> cannot be initialized, so move all instances out of the switches.
> After this, future always-initialized stack variables will work
> and not throw warnings like this:
> 
> fs/fcntl.c: In function ‘send_sigio_to_task’:
> fs/fcntl.c:738:13: warning: statement will never be executed 
> [-Wswitch-unreachable]
>siginfo_t si;
>  ^~
> 
> Signed-off-by: Kees Cook 

Reviewed by: Edwin Zimmerman 

> ---
>  arch/x86/xen/enlighten_pv.c   |  7 ---
>  drivers/char/pcmcia/cm4000_cs.c   |  2 +-
>  drivers/char/ppdev.c  | 20 ---
>  drivers/gpu/drm/drm_edid.c|  4 ++--
>  drivers/gpu/drm/i915/intel_display.c  |  2 +-
>  drivers/gpu/drm/i915/intel_pm.c   |  4 ++--
>  drivers/net/ethernet/intel/e1000/e1000_main.c |  3 ++-
>  drivers/tty/n_tty.c   |  3 +--
>  drivers/usb/gadget/udc/net2280.c  |  5 ++---
>  fs/fcntl.c|  3 ++-
>  mm/shmem.c|  5 +++--
>  net/core/skbuff.c |  4 ++--
>  net/ipv6/ip6_gre.c|  4 ++--
>  net/ipv6/ip6_tunnel.c |  4 ++--
>  net/openvswitch/flow_netlink.c|  7 +++
>  security/tomoyo/common.c  |  3 ++-
>  security/tomoyo/condition.c   |  7 ---
>  security/tomoyo/util.c|  4 ++--
>  18 files changed, 45 insertions(+), 46 deletions(-)
> 
> diff --git a/arch/x86/xen/enlighten_pv.c b/arch/x86/xen/enlighten_pv.c
> index c54a493e139a..a79d4b548a08 100644
> --- a/arch/x86/xen/enlighten_pv.c
> +++ b/arch/x86/xen/enlighten_pv.c
> @@ -907,14 +907,15 @@ static u64 xen_read_msr_safe(unsigned int msr, int *err)
>  static int xen_write_msr_safe(unsigned int msr, unsigned low, unsigned high)
>  {
>   int ret;
> +#ifdef CONFIG_X86_64
> + unsigned which;
> + u64 base;
> +#endif
> 
>   ret = 0;
> 
>   switch (msr) {
>  #ifdef CONFIG_X86_64
> - unsigned which;
> - u64 base;
> -
>   case MSR_FS_BASE:   which = SEGBASE_FS; goto set;
>   case MSR_KERNEL_GS_BASE:which = SEGBASE_GS_USER; goto set;
>   case MSR_GS_BASE:   which = SEGBASE_GS_KERNEL; goto set;
> diff --git a/drivers/char/pcmcia/cm4000_cs.c b/drivers/char/pcmcia/cm4000_cs.c
> index 7a4eb86aedac..7211dc0e6f4f 100644
> --- a/drivers/char/pcmcia/cm4000_cs.c
> +++ b/drivers/char/pcmcia/cm4000_cs.c
> @@ -663,6 +663,7 @@ static void monitor_card(struct timer_list *t)
>  {
>   struct cm4000_dev *dev = from_timer(dev, t, timer);
>   unsigned int iobase = dev->p_dev->resource[0]->start;
> + unsigned char flags0;
>   unsigned short s;
>   struct ptsreq ptsreq;
>   int i, atrc;
> @@ -731,7 +732,6 @@ static void monitor_card(struct timer_list *t)
>   }
> 
>   switch (dev->mstate) {
> - unsigned char flags0;
>   case M_CARDOFF:
>   DEBUGP(4, dev, "M_CARDOFF\n");
>   flags0 = inb(REG_FLAGS0(iobase));
> diff --git a/drivers/char/ppdev.c b/drivers/char/ppdev.c
> index 1ae77b41050a..d77c97e4f996 100644
> --- a/drivers/char/ppdev.c
> +++ b/drivers/char/ppdev.c
> @@ -359,14 +359,19 @@ static int pp_do_ioctl(struct file *file, unsigned int 
> cmd, unsigned long arg)
>   struct pp_struct *pp = file->private_data;
>   struct parport *port;
>   void __user *argp = (void __user *)arg;
> + struct ieee1284_info *info;
> + unsigned char reg;
> + unsigned char mask;
> + int mode;
> + s32 time32[2];
> + s64 time64[2];
> + struct timespec64 ts;
> + int ret;
> 
>   /* First handle the cases that don't take arguments. */
>   switch (cmd) {
>   case PPCLAIM:
>   {
> - struct ieee1284_info *info;
> - int ret;
> -
>   if (pp->flags & PP_CLAIMED) {
>   dev_dbg(>pdev->dev, "you've already got it!\n");
>   return -EINVAL;
> @@ -517,15 +522,6 @@ static int pp_do_ioctl(struct file *file, unsigned int 
> cmd, unsigned long arg)
> 
>   port = pp->pdev->port;
>   switch (cmd) {
> - struct ieee1284_info *info;
> - unsigned char reg;
> - unsigned char mask;
> - int mode;
> - s32 time32[2];
> - s64 time64[2];
> -   

RE: [Intel-gfx] [PATCH 1/3] treewide: Lift switch variables out of switches

2019-01-24 Thread Edwin Zimmerman
On Wed, 23 Jan 2019, Jani Nikula  wrote:
> On Wed, 23 Jan 2019, Greg KH  wrote:
> > On Wed, Jan 23, 2019 at 03:03:47AM -0800, Kees Cook wrote:
> >> Variables declared in a switch statement before any case statements
> >> cannot be initialized, so move all instances out of the switches.
> >> After this, future always-initialized stack variables will work
> >> and not throw warnings like this:
> >>
> >> fs/fcntl.c: In function ‘send_sigio_to_task’:
> >> fs/fcntl.c:738:13: warning: statement will never be executed 
> >> [-Wswitch-unreachable]
> >>siginfo_t si;
> >>  ^~
> >
> > That's a pain, so this means we can't have any new variables in { }
> > scope except for at the top of a function?
> >
> > That's going to be a hard thing to keep from happening over time, as
> > this is valid C :(
> 
> Not all valid C is meant to be used! ;)

Very true.  The other thing to keep in mind is the burden of enforcing a 
prohibition on a valid C construct like this.  
It seems to me that patch reviewers and maintainers have enough to do without 
forcing them to watch for variable
declarations in switch statements.  Automating this prohibition, should it be 
accepted, seems like a good idea to me.

-Edwin Zimmerman

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel