[03/16] lirc_dev: correct error handling
diff mbox

Message ID 149365463117.12922.15518669536847504845.stgit@zeus.hardeman.nu
State New
Headers show

Commit Message

David Härdeman May 1, 2017, 4:03 p.m. UTC
If an error is generated, nonseekable_open() shouldn't be called.

Signed-off-by: David Härdeman <david@hardeman.nu>
---
 drivers/media/rc/lirc_dev.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

Comments

Sean Young May 21, 2017, 8:57 a.m. UTC | #1
On Mon, May 01, 2017 at 06:03:51PM +0200, David Härdeman wrote:
> If an error is generated, nonseekable_open() shouldn't be called.

There is no harm in calling nonseekable_open(), so this commit is
misleading.

Sean

> 
> Signed-off-by: David Härdeman <david@hardeman.nu>
> ---
>  drivers/media/rc/lirc_dev.c |    6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/rc/lirc_dev.c b/drivers/media/rc/lirc_dev.c
> index 05f600bd6c67..7f13ed479e1c 100644
> --- a/drivers/media/rc/lirc_dev.c
> +++ b/drivers/media/rc/lirc_dev.c
> @@ -431,7 +431,7 @@ EXPORT_SYMBOL(lirc_unregister_driver);
>  int lirc_dev_fop_open(struct inode *inode, struct file *file)
>  {
>  	struct irctl *ir;
> -	int retval = 0;
> +	int retval;
>  
>  	if (iminor(inode) >= MAX_IRCTL_DEVICES) {
>  		pr_err("open result for %d is -ENODEV\n", iminor(inode));
> @@ -475,9 +475,11 @@ int lirc_dev_fop_open(struct inode *inode, struct file *file)
>  
>  	ir->open++;
>  
> -error:
>  	nonseekable_open(inode, file);
>  
> +	return 0;
> +
> +error:
>  	return retval;
>  }
>  EXPORT_SYMBOL(lirc_dev_fop_open);
David Härdeman May 28, 2017, 8:23 a.m. UTC | #2
On Sun, May 21, 2017 at 09:57:13AM +0100, Sean Young wrote:
>On Mon, May 01, 2017 at 06:03:51PM +0200, David Härdeman wrote:
>> If an error is generated, nonseekable_open() shouldn't be called.
>
>There is no harm in calling nonseekable_open(), so this commit is
>misleading.

I'm not sure why you consider it misleading? If there's an error, the
logic thing to do is to error out immediately and not do any further
work?

>> Signed-off-by: David Härdeman <david@hardeman.nu>
>> ---
>>  drivers/media/rc/lirc_dev.c |    6 ++++--
>>  1 file changed, 4 insertions(+), 2 deletions(-)
>> 
>> diff --git a/drivers/media/rc/lirc_dev.c b/drivers/media/rc/lirc_dev.c
>> index 05f600bd6c67..7f13ed479e1c 100644
>> --- a/drivers/media/rc/lirc_dev.c
>> +++ b/drivers/media/rc/lirc_dev.c
>> @@ -431,7 +431,7 @@ EXPORT_SYMBOL(lirc_unregister_driver);
>>  int lirc_dev_fop_open(struct inode *inode, struct file *file)
>>  {
>>  	struct irctl *ir;
>> -	int retval = 0;
>> +	int retval;
>>  
>>  	if (iminor(inode) >= MAX_IRCTL_DEVICES) {
>>  		pr_err("open result for %d is -ENODEV\n", iminor(inode));
>> @@ -475,9 +475,11 @@ int lirc_dev_fop_open(struct inode *inode, struct file *file)
>>  
>>  	ir->open++;
>>  
>> -error:
>>  	nonseekable_open(inode, file);
>>  
>> +	return 0;
>> +
>> +error:
>>  	return retval;
>>  }
>>  EXPORT_SYMBOL(lirc_dev_fop_open);
Sean Young May 28, 2017, 3:04 p.m. UTC | #3
On Sun, May 28, 2017 at 10:23:37AM +0200, David Härdeman wrote:
> On Sun, May 21, 2017 at 09:57:13AM +0100, Sean Young wrote:
> >On Mon, May 01, 2017 at 06:03:51PM +0200, David Härdeman wrote:
> >> If an error is generated, nonseekable_open() shouldn't be called.
> >
> >There is no harm in calling nonseekable_open(), so this commit is
> >misleading.
> 
> I'm not sure why you consider it misleading? If there's an error, the
> logic thing to do is to error out immediately and not do any further
> work?

The commit message says that nonseekable_open() should not be called,
suggesting there is a bug which is not the case.


Sean
David Härdeman June 17, 2017, 11:14 a.m. UTC | #4
On Sun, May 28, 2017 at 04:04:30PM +0100, Sean Young wrote:
>On Sun, May 28, 2017 at 10:23:37AM +0200, David Härdeman wrote:
>> On Sun, May 21, 2017 at 09:57:13AM +0100, Sean Young wrote:
>> >On Mon, May 01, 2017 at 06:03:51PM +0200, David Härdeman wrote:
>> >> If an error is generated, nonseekable_open() shouldn't be called.
>> >
>> >There is no harm in calling nonseekable_open(), so this commit is
>> >misleading.
>> 
>> I'm not sure why you consider it misleading? If there's an error, the
>> logic thing to do is to error out immediately and not do any further
>> work?
>
>The commit message says that nonseekable_open() should not be called,
>suggesting there is a bug which is not the case.

I'll do another version with an updated commit message then :)

Patch
diff mbox

diff --git a/drivers/media/rc/lirc_dev.c b/drivers/media/rc/lirc_dev.c
index 05f600bd6c67..7f13ed479e1c 100644
--- a/drivers/media/rc/lirc_dev.c
+++ b/drivers/media/rc/lirc_dev.c
@@ -431,7 +431,7 @@  EXPORT_SYMBOL(lirc_unregister_driver);
 int lirc_dev_fop_open(struct inode *inode, struct file *file)
 {
 	struct irctl *ir;
-	int retval = 0;
+	int retval;
 
 	if (iminor(inode) >= MAX_IRCTL_DEVICES) {
 		pr_err("open result for %d is -ENODEV\n", iminor(inode));
@@ -475,9 +475,11 @@  int lirc_dev_fop_open(struct inode *inode, struct file *file)
 
 	ir->open++;
 
-error:
 	nonseekable_open(inode, file);
 
+	return 0;
+
+error:
 	return retval;
 }
 EXPORT_SYMBOL(lirc_dev_fop_open);