linux-kernel - Re: [PATCH 2/3] staging: panel: return register value

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] [day] [month] [year] [list]

Message-ID: <20150309150500.GB6206@1wt.eu>
Date:	Mon, 9 Mar 2015 16:05:00 +0100
From:	Willy Tarreau <w@....eu>
To:	Sudip Mukherjee <sudipm.mukherjee@...il.com>
Cc:	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	devel@...verdev.osuosl.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/3] staging: panel: return register value

On Mon, Mar 09, 2015 at 07:05:59PM +0530, Sudip Mukherjee wrote:
> On Mon, Mar 09, 2015 at 07:30:07AM +0100, Willy Tarreau wrote:
> > On Sun, Mar 08, 2015 at 11:07:25PM +0530, Sudip Mukherjee wrote:
> > >  
> > > -	if (parport_register_driver(&panel_driver)) {
> > > -		pr_err("could not register with parport. Aborting.\n");
> > > -		return -EIO;
> > > -	}
> > > -
> > >  	if (pprt)
> > >  		pr_info("driver version " PANEL_VERSION
> > >  			" registered on parport%d (io=0x%lx).\n", parport,
> > > @@ -2375,7 +2370,7 @@ static int __init panel_init_module(void)
> > >  	/* tells various subsystems about the fact that initialization
> > >  	   is finished */
> > >  	init_in_progress = 0;
> > > -	return 0;
> > > +	return parport_register_driver(&panel_driver);
> > 
> > Here I'd rather keep the error message, as it's quite annoying when a
> > driver does not load and you don't know why, especially with something
> > like parport where there are many possible causes. Something like this
> > would be better in my opinion :
> > 
> > -	return 0;
> > +	err = parport_register_driver(&panel_driver);
> > +	if (err)
> > +		pr_err("could not register with parport. Aborting.\n");
> > +	return err;
> > 
> > Well, I just found that currently parport_register_driver() always
> > succeeds, but I still think it's better to verbosely handle errors.
> 
> ok, i will send a v2, but is the error message really required? considering
> the fact that parport_register_driver() always succeeds ..

It's a matter of choice :
   - either we consider that it can fail and we emit an appropriate
     error message ;

   - or we consider it cannot fail and instead of returning its own
     code we always call it without checking it then return zero. That
     way if it were ever to change, it would be easy to spot the change.

I still find that the first choice is appropriate, comes with a very low
cost and is in line with what the parrallel port driver does as well.

Regards,
Willy

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/