linux-kernel - Re: [PATCH v4 3/3] axis-fifo: cleanup space issues with fops struct

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20230527223111.jidik3ffcsxdkenb@pengutronix.de>
Date:   Sun, 28 May 2023 00:31:11 +0200
From:   Uwe Kleine-König <u.kleine-koenig@...gutronix.de>
To:     Prathu Baronia <prathubaronia2011@...il.com>
Cc:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        "Fabio M. De Francesco" <fmdefrancesco@...il.com>,
        Khadija Kamran <kamrankhadijadj@...il.com>,
        linux-staging@...ts.linux.dev, linux-kernel@...r.kernel.org,
        dan.carpenter@...aro.org, error27@...il.com, lkp@...el.com,
        oe-kbuild-all@...ts.linux.dev, oe-kbuild@...ts.linux.dev
Subject: Re: [PATCH v4 3/3] axis-fifo: cleanup space issues with fops struct

Hello,

On Sat, May 27, 2023 at 05:21:00PM +0530, Prathu Baronia wrote:
> Add required spaces for proper formatting of fops members for better
> readability.
> 
> Signed-off-by: Prathu Baronia <prathubaronia2011@...il.com>
> ---
>  drivers/staging/axis-fifo/axis-fifo.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/staging/axis-fifo/axis-fifo.c b/drivers/staging/axis-fifo/axis-fifo.c
> index d71bdc6dd961..59e962467a3d 100644
> --- a/drivers/staging/axis-fifo/axis-fifo.c
> +++ b/drivers/staging/axis-fifo/axis-fifo.c
> @@ -716,11 +716,11 @@ static int axis_fifo_close(struct inode *inod, struct file *f)
>  }
>  
>  static const struct file_operations fops = {
> -	.owner = THIS_MODULE,
> -	.open = axis_fifo_open,
> +	.owner   = THIS_MODULE,
> +	.open    = axis_fifo_open,
>  	.release = axis_fifo_close,
> -	.read = axis_fifo_read,
> -	.write = axis_fifo_write
> +	.read    = axis_fifo_read,
> +	.write   = axis_fifo_write

Note this is only subjectively better. IMHO with just a single space
this is perfectly readable. Aligning the = might look nice, but it's
also annoying at times. When you add another member (e.g.
.iterate_shared) you either add a line that doesn't match all others, or
you have to touch all other lines of that struct which (objectively?)
hurts readability of that patch. Also for generated patches this kind of
alignment yields extra work. (See for example
https://lore.kernel.org/lkml/20230525205840.734432-1-u.kleine-koenig@pengutronix.de/
which required semi-manual fixup to keep the alignment after coccinelle
generated the patch.)

If you still think this is a good idea, I'd ask you to stick to one
style for the whole file. e.g. axis_fifo_driver uses inconsistent
and different indention.

A thing that IMHO is more useful to change here, is the name fops; I'd
suggest something like axis_fifo_fops (and also use prefixes for some
other symbols like "get_dts_property"). In 6.4-rc1 my ctags file knows
about 64 different places that define something called "fops".

Just my 0.02€,
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)