[OPW,kernel] Fix checkpatch.pl issues with seq_puts is prefered to seq_printf in comedi/proc.c
diff mbox

Message ID 1381093124-19566-1-git-send-email-maria.solher@gmail.com
State Rejected
Headers show

Commit Message

Maria Soler Heredia Oct. 6, 2013, 8:58 p.m. UTC
Signed-off-by: Maria Soler Heredia <maria.solher@gmail.com>
---
 drivers/staging/comedi/proc.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Comments

Josh Triplett Oct. 6, 2013, 9:31 p.m. UTC | #1
On Sun, Oct 06, 2013 at 10:58:44PM +0200, Maria Soler Heredia wrote:
> Signed-off-by: Maria Soler Heredia <maria.solher@gmail.com>
> ---
>  drivers/staging/comedi/proc.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/staging/comedi/proc.c b/drivers/staging/comedi/proc.c
> index ade0003..c538ed6 100644
> --- a/drivers/staging/comedi/proc.c
> +++ b/drivers/staging/comedi/proc.c
> @@ -34,11 +34,11 @@ static int comedi_read(struct seq_file *m, void *v)
>  	int devices_q = 0;
>  	struct comedi_driver *driv;
>  
> -	seq_printf(m,
> +	seq_puts(m,
>  		     "comedi version " COMEDI_RELEASE "\n"
> -		     "format string: %s\n",
> +		     "format string: "
>  		     "\"%2d: %-20s %-20s %4d\", i, "
> -		     "driver_name, board_name, n_subdevices");
> +		     "driver_name, board_name, n_subdevices\n");

I had to stare at this several times over.  It's fairly absurd, but your
patch looks correct, and makes it marginally less absurd. :)

Reviewed-by: Josh Triplett <josh@joshtriplett.org>
Greg Kroah-Hartman Oct. 7, 2013, 5:52 a.m. UTC | #2
On Sun, Oct 06, 2013 at 10:58:44PM +0200, Maria Soler Heredia wrote:
> Signed-off-by: Maria Soler Heredia <maria.solher@gmail.com>
> ---
>  drivers/staging/comedi/proc.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/staging/comedi/proc.c b/drivers/staging/comedi/proc.c
> index ade0003..c538ed6 100644
> --- a/drivers/staging/comedi/proc.c
> +++ b/drivers/staging/comedi/proc.c
> @@ -34,11 +34,11 @@ static int comedi_read(struct seq_file *m, void *v)
>  	int devices_q = 0;
>  	struct comedi_driver *driv;
>  
> -	seq_printf(m,
> +	seq_puts(m,
>  		     "comedi version " COMEDI_RELEASE "\n"
> -		     "format string: %s\n",
> +		     "format string: "
>  		     "\"%2d: %-20s %-20s %4d\", i, "

I don't think this is correct, as you dropped the format string, why?

Also seq_printf() is just fine here, it shouldn't be changed.  I think
checkpatch is incorrect most of the time it says things like this,
sorry.

greg k-h
Greg Kroah-Hartman Oct. 7, 2013, 5:54 a.m. UTC | #3
On Sun, Oct 06, 2013 at 02:31:07PM -0700, Josh Triplett wrote:
> On Sun, Oct 06, 2013 at 10:58:44PM +0200, Maria Soler Heredia wrote:
> > Signed-off-by: Maria Soler Heredia <maria.solher@gmail.com>
> > ---
> >  drivers/staging/comedi/proc.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/staging/comedi/proc.c b/drivers/staging/comedi/proc.c
> > index ade0003..c538ed6 100644
> > --- a/drivers/staging/comedi/proc.c
> > +++ b/drivers/staging/comedi/proc.c
> > @@ -34,11 +34,11 @@ static int comedi_read(struct seq_file *m, void *v)
> >  	int devices_q = 0;
> >  	struct comedi_driver *driv;
> >  
> > -	seq_printf(m,
> > +	seq_puts(m,
> >  		     "comedi version " COMEDI_RELEASE "\n"
> > -		     "format string: %s\n",
> > +		     "format string: "
> >  		     "\"%2d: %-20s %-20s %4d\", i, "
> > -		     "driver_name, board_name, n_subdevices");
> > +		     "driver_name, board_name, n_subdevices\n");
> 
> I had to stare at this several times over.  It's fairly absurd, but your
> patch looks correct, and makes it marginally less absurd. :)

It is correct?  I don't think so, the format string is now being
interrupted where before it wasn't, right?

ick, what a mess, I say just leave this thing alone...

greg k-h
Josh Triplett Oct. 7, 2013, 7 a.m. UTC | #4
On Sun, Oct 06, 2013 at 10:54:15PM -0700, Greg KH wrote:
> On Sun, Oct 06, 2013 at 02:31:07PM -0700, Josh Triplett wrote:
> > On Sun, Oct 06, 2013 at 10:58:44PM +0200, Maria Soler Heredia wrote:
> > > Signed-off-by: Maria Soler Heredia <maria.solher@gmail.com>
> > > ---
> > >  drivers/staging/comedi/proc.c | 6 +++---
> > >  1 file changed, 3 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/staging/comedi/proc.c b/drivers/staging/comedi/proc.c
> > > index ade0003..c538ed6 100644
> > > --- a/drivers/staging/comedi/proc.c
> > > +++ b/drivers/staging/comedi/proc.c
> > > @@ -34,11 +34,11 @@ static int comedi_read(struct seq_file *m, void *v)
> > >  	int devices_q = 0;
> > >  	struct comedi_driver *driv;
> > >  
> > > -	seq_printf(m,
> > > +	seq_puts(m,
> > >  		     "comedi version " COMEDI_RELEASE "\n"
> > > -		     "format string: %s\n",
> > > +		     "format string: "
> > >  		     "\"%2d: %-20s %-20s %4d\", i, "
> > > -		     "driver_name, board_name, n_subdevices");
> > > +		     "driver_name, board_name, n_subdevices\n");
> > 
> > I had to stare at this several times over.  It's fairly absurd, but your
> > patch looks correct, and makes it marginally less absurd. :)
> 
> It is correct?  I don't think so, the format string is now being
> interrupted where before it wasn't, right?

Note that the original version is using %s to print a literal format
string and series of arguments as a quoted string, not actually passing
those arguments to seq_printf.  The new version, using seq_puts, just
directly substitutes in that string in place of the %s, moving the \n to
the end.  They have exactly the same effect.

- Josh Triplett
Greg Kroah-Hartman Oct. 7, 2013, 7:20 a.m. UTC | #5
On Mon, Oct 07, 2013 at 12:00:32AM -0700, Josh Triplett wrote:
> On Sun, Oct 06, 2013 at 10:54:15PM -0700, Greg KH wrote:
> > On Sun, Oct 06, 2013 at 02:31:07PM -0700, Josh Triplett wrote:
> > > On Sun, Oct 06, 2013 at 10:58:44PM +0200, Maria Soler Heredia wrote:
> > > > Signed-off-by: Maria Soler Heredia <maria.solher@gmail.com>
> > > > ---
> > > >  drivers/staging/comedi/proc.c | 6 +++---
> > > >  1 file changed, 3 insertions(+), 3 deletions(-)
> > > > 
> > > > diff --git a/drivers/staging/comedi/proc.c b/drivers/staging/comedi/proc.c
> > > > index ade0003..c538ed6 100644
> > > > --- a/drivers/staging/comedi/proc.c
> > > > +++ b/drivers/staging/comedi/proc.c
> > > > @@ -34,11 +34,11 @@ static int comedi_read(struct seq_file *m, void *v)
> > > >  	int devices_q = 0;
> > > >  	struct comedi_driver *driv;
> > > >  
> > > > -	seq_printf(m,
> > > > +	seq_puts(m,
> > > >  		     "comedi version " COMEDI_RELEASE "\n"
> > > > -		     "format string: %s\n",
> > > > +		     "format string: "
> > > >  		     "\"%2d: %-20s %-20s %4d\", i, "
> > > > -		     "driver_name, board_name, n_subdevices");
> > > > +		     "driver_name, board_name, n_subdevices\n");
> > > 
> > > I had to stare at this several times over.  It's fairly absurd, but your
> > > patch looks correct, and makes it marginally less absurd. :)
> > 
> > It is correct?  I don't think so, the format string is now being
> > interrupted where before it wasn't, right?
> 
> Note that the original version is using %s to print a literal format
> string and series of arguments as a quoted string, not actually passing
> those arguments to seq_printf.  The new version, using seq_puts, just
> directly substitutes in that string in place of the %s, moving the \n to
> the end.  They have exactly the same effect.

Ah, what a mess.  The string really shouldn't be broken across two lines
if it's the same string, that confused me.

I recommend leaving it alone, the comedi developers are quite active, if
they want to clean it up, let them do it :)

thanks,

greg k-h
Maria Soler Heredia Oct. 7, 2013, 7:29 a.m. UTC | #6
I have switched my attention to the driver/s under the android folder. I
think it may be a better option.

Thank you both for your advice,

María.


On 7 October 2013 09:20, Greg KH <gregkh@linuxfoundation.org> wrote:

> On Mon, Oct 07, 2013 at 12:00:32AM -0700, Josh Triplett wrote:
> > On Sun, Oct 06, 2013 at 10:54:15PM -0700, Greg KH wrote:
> > > On Sun, Oct 06, 2013 at 02:31:07PM -0700, Josh Triplett wrote:
> > > > On Sun, Oct 06, 2013 at 10:58:44PM +0200, Maria Soler Heredia wrote:
> > > > > Signed-off-by: Maria Soler Heredia <maria.solher@gmail.com>
> > > > > ---
> > > > >  drivers/staging/comedi/proc.c | 6 +++---
> > > > >  1 file changed, 3 insertions(+), 3 deletions(-)
> > > > >
> > > > > diff --git a/drivers/staging/comedi/proc.c
> b/drivers/staging/comedi/proc.c
> > > > > index ade0003..c538ed6 100644
> > > > > --- a/drivers/staging/comedi/proc.c
> > > > > +++ b/drivers/staging/comedi/proc.c
> > > > > @@ -34,11 +34,11 @@ static int comedi_read(struct seq_file *m,
> void *v)
> > > > >         int devices_q = 0;
> > > > >         struct comedi_driver *driv;
> > > > >
> > > > > -       seq_printf(m,
> > > > > +       seq_puts(m,
> > > > >                      "comedi version " COMEDI_RELEASE "\n"
> > > > > -                    "format string: %s\n",
> > > > > +                    "format string: "
> > > > >                      "\"%2d: %-20s %-20s %4d\", i, "
> > > > > -                    "driver_name, board_name, n_subdevices");
> > > > > +                    "driver_name, board_name, n_subdevices\n");
> > > >
> > > > I had to stare at this several times over.  It's fairly absurd, but
> your
> > > > patch looks correct, and makes it marginally less absurd. :)
> > >
> > > It is correct?  I don't think so, the format string is now being
> > > interrupted where before it wasn't, right?
> >
> > Note that the original version is using %s to print a literal format
> > string and series of arguments as a quoted string, not actually passing
> > those arguments to seq_printf.  The new version, using seq_puts, just
> > directly substitutes in that string in place of the %s, moving the \n to
> > the end.  They have exactly the same effect.
>
> Ah, what a mess.  The string really shouldn't be broken across two lines
> if it's the same string, that confused me.
>
> I recommend leaving it alone, the comedi developers are quite active, if
> they want to clean it up, let them do it :)
>
> thanks,
>
> greg k-h
>
Josh Triplett Oct. 7, 2013, 9:53 a.m. UTC | #7
On Mon, Oct 07, 2013 at 12:20:37AM -0700, Greg KH wrote:
> On Mon, Oct 07, 2013 at 12:00:32AM -0700, Josh Triplett wrote:
> > On Sun, Oct 06, 2013 at 10:54:15PM -0700, Greg KH wrote:
> > > On Sun, Oct 06, 2013 at 02:31:07PM -0700, Josh Triplett wrote:
> > > > On Sun, Oct 06, 2013 at 10:58:44PM +0200, Maria Soler Heredia wrote:
> > > > > Signed-off-by: Maria Soler Heredia <maria.solher@gmail.com>
> > > > > ---
> > > > >  drivers/staging/comedi/proc.c | 6 +++---
> > > > >  1 file changed, 3 insertions(+), 3 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/staging/comedi/proc.c b/drivers/staging/comedi/proc.c
> > > > > index ade0003..c538ed6 100644
> > > > > --- a/drivers/staging/comedi/proc.c
> > > > > +++ b/drivers/staging/comedi/proc.c
> > > > > @@ -34,11 +34,11 @@ static int comedi_read(struct seq_file *m, void *v)
> > > > >  	int devices_q = 0;
> > > > >  	struct comedi_driver *driv;
> > > > >  
> > > > > -	seq_printf(m,
> > > > > +	seq_puts(m,
> > > > >  		     "comedi version " COMEDI_RELEASE "\n"
> > > > > -		     "format string: %s\n",
> > > > > +		     "format string: "
> > > > >  		     "\"%2d: %-20s %-20s %4d\", i, "
> > > > > -		     "driver_name, board_name, n_subdevices");
> > > > > +		     "driver_name, board_name, n_subdevices\n");
> > > > 
> > > > I had to stare at this several times over.  It's fairly absurd, but your
> > > > patch looks correct, and makes it marginally less absurd. :)
> > > 
> > > It is correct?  I don't think so, the format string is now being
> > > interrupted where before it wasn't, right?
> > 
> > Note that the original version is using %s to print a literal format
> > string and series of arguments as a quoted string, not actually passing
> > those arguments to seq_printf.  The new version, using seq_puts, just
> > directly substitutes in that string in place of the %s, moving the \n to
> > the end.  They have exactly the same effect.
> 
> Ah, what a mess.  The string really shouldn't be broken across two lines
> if it's the same string, that confused me.

Agreed completely, and since that's another checkpatch cleanup, it seems
worth doing.

> I recommend leaving it alone, the comedi developers are quite active, if
> they want to clean it up, let them do it :)

It's your call, but this seems like a worthwhile improvement worth
applying, and it makes the print quite a bit less complicated.  (Joining
the strings would also help, but that doesn't have to happen in the same
commit.)

- Josh Triplett
Greg Kroah-Hartman Oct. 7, 2013, 8:21 p.m. UTC | #8
On Mon, Oct 07, 2013 at 09:29:12AM +0200, María Soler Heredia wrote:
> I have switched my attention to the driver/s under the android folder. I think
> it may be a better option.

The android code is fragile, fickle, and should be all cleaned up by
now, I don't think there is any "easy" fixes left there.

One place I would recommend for everyone is drivers/staging/lustre/
There's a _ton_ of work to be done there, and almost every file needs
major cleanup work, so there should be plenty of easy stuff to start
with.

Hope this helps,

greg k-h

Patch
diff mbox

diff --git a/drivers/staging/comedi/proc.c b/drivers/staging/comedi/proc.c
index ade0003..c538ed6 100644
--- a/drivers/staging/comedi/proc.c
+++ b/drivers/staging/comedi/proc.c
@@ -34,11 +34,11 @@  static int comedi_read(struct seq_file *m, void *v)
 	int devices_q = 0;
 	struct comedi_driver *driv;
 
-	seq_printf(m,
+	seq_puts(m,
 		     "comedi version " COMEDI_RELEASE "\n"
-		     "format string: %s\n",
+		     "format string: "
 		     "\"%2d: %-20s %-20s %4d\", i, "
-		     "driver_name, board_name, n_subdevices");
+		     "driver_name, board_name, n_subdevices\n");
 
 	for (i = 0; i < COMEDI_NUM_BOARD_MINORS; i++) {
 		struct comedi_device *dev = comedi_dev_from_minor(i);