[PATCH] isdn divas: fix proc creation

[PATCH] isdn divas: fix proc creation

[Alexey Dobriyan]

1. creating proc entry and not saving pointer to PDE and checking it
   is not going to work.
2. if proc entry wasn't created, no reason to remove it on error path.

Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
---

 drivers/isdn/hardware/eicon/divasmain.c |    1 -
 drivers/isdn/hardware/eicon/divasproc.c |    4 ++--
 2 files changed, 2 insertions(+), 3 deletions(-)

--- a/drivers/isdn/hardware/eicon/divasmain.c
+++ b/drivers/isdn/hardware/eicon/divasmain.c
@@ -806,7 +806,6 @@ static int DIVA_INIT_FUNCTION divas_init(void)
 
 	if (!create_divas_proc()) {
 #ifdef MODULE
-		remove_divas_proc();
 		divas_unregister_chrdev();
 		divasfunc_exit();
 #endif
--- a/drivers/isdn/hardware/eicon/divasproc.c
+++ b/drivers/isdn/hardware/eicon/divasproc.c
@@ -125,8 +125,8 @@ static const struct file_operations divas_fops = {
 
 int create_divas_proc(void)
 {
-	proc_create(divas_proc_name, S_IFREG | S_IRUGO, proc_net_eicon,
-		    &divas_fops);
+	divas_proc_entry = proc_create(divas_proc_name, S_IFREG | S_IRUGO,
+					proc_net_eicon, &divas_fops);
 	if (!divas_proc_entry)
 		return (0);
 

Re: [PATCH] isdn divas: fix proc creation

[Armin Schindler]

On Sat, 31 May 2008, Alexey Dobriyan wrote:
> 1. creating proc entry and not saving pointer to PDE and checking it
>   is not going to work.

I don't know where you found this. I have look even in older versions, but
the pointer divas_proc_entry is set by proc_create(). The patch to
divasproc.c is wrong, it exists from the beginning of the driver.
(2.6.25.4 doesn't contain the bug you describe).

> 2. if proc entry wasn't created, no reason to remove it on error path.

Agreed here. The reason for doing remove_divas_proc() anyway, was to
make sure all possible settings done in !create_divas_proc() are
reverted. But since nothing else is done here, it is okay to remove it.

Armin

> Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
> ---
>
> drivers/isdn/hardware/eicon/divasmain.c |    1 -
> drivers/isdn/hardware/eicon/divasproc.c |    4 ++--
> 2 files changed, 2 insertions(+), 3 deletions(-)
>
> --- a/drivers/isdn/hardware/eicon/divasmain.c
> +++ b/drivers/isdn/hardware/eicon/divasmain.c
> @@ -806,7 +806,6 @@ static int DIVA_INIT_FUNCTION divas_init(void)
>
> 	if (!create_divas_proc()) {
> #ifdef MODULE
> -		remove_divas_proc();
> 		divas_unregister_chrdev();
> 		divasfunc_exit();
> #endif
> --- a/drivers/isdn/hardware/eicon/divasproc.c
> +++ b/drivers/isdn/hardware/eicon/divasproc.c
> @@ -125,8 +125,8 @@ static const struct file_operations divas_fops = {
>
> int create_divas_proc(void)
> {
> -	proc_create(divas_proc_name, S_IFREG | S_IRUGO, proc_net_eicon,
> -		    &divas_fops);
> +	divas_proc_entry = proc_create(divas_proc_name, S_IFREG | S_IRUGO,
> +					proc_net_eicon, &divas_fops);
> 	if (!divas_proc_entry)
> 		return (0);
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>

Re: [PATCH] isdn divas: fix proc creation

[Alexey Dobriyan]

On Thu, Jun 05, 2008 at 12:25:06PM +0200, Armin Schindler wrote:
> On Sat, 31 May 2008, Alexey Dobriyan wrote:
>> 1. creating proc entry and not saving pointer to PDE and checking it
>>   is not going to work.
>
> I don't know where you found this. I have look even in older versions, but
> the pointer divas_proc_entry is set by proc_create(). The patch to
> divasproc.c is wrong, it exists from the beginning of the driver.
> (2.6.25.4 doesn't contain the bug you describe).

Check mainline kernel, namely, 2.6.26-rc5.

>> --- a/drivers/isdn/hardware/eicon/divasproc.c
>> +++ b/drivers/isdn/hardware/eicon/divasproc.c
>> @@ -125,8 +125,8 @@ static const struct file_operations divas_fops = {
>>
>> int create_divas_proc(void)
>> {
>> -	proc_create(divas_proc_name, S_IFREG | S_IRUGO, proc_net_eicon,
>> -		    &divas_fops);
>> +	divas_proc_entry = proc_create(divas_proc_name, S_IFREG | S_IRUGO,
>> +					proc_net_eicon, &divas_fops);
>> 	if (!divas_proc_entry)
>> 		return (0);

Re: [PATCH] isdn divas: fix proc creation

[Armin Schindler]

On Fri, 6 Jun 2008, Alexey Dobriyan wrote:
> On Thu, Jun 05, 2008 at 12:25:06PM +0200, Armin Schindler wrote:
>> On Sat, 31 May 2008, Alexey Dobriyan wrote:
>>> 1. creating proc entry and not saving pointer to PDE and checking it
>>>   is not going to work.
>>
>> I don't know where you found this. I have look even in older versions, but
>> the pointer divas_proc_entry is set by proc_create(). The patch to
>> divasproc.c is wrong, it exists from the beginning of the driver.
>> (2.6.25.4 doesn't contain the bug you describe).
>
> Check mainline kernel, namely, 2.6.26-rc5.

Ah, okay. So someone removed the pointer between 2.6.25 and 2.6.26 then.
In that case your patch is correct of course.

Armin

>>> --- a/drivers/isdn/hardware/eicon/divasproc.c
>>> +++ b/drivers/isdn/hardware/eicon/divasproc.c
>>> @@ -125,8 +125,8 @@ static const struct file_operations divas_fops = {
>>>
>>> int create_divas_proc(void)
>>> {
>>> -	proc_create(divas_proc_name, S_IFREG | S_IRUGO, proc_net_eicon,
>>> -		    &divas_fops);
>>> +	divas_proc_entry = proc_create(divas_proc_name, S_IFREG | S_IRUGO,
>>> +					proc_net_eicon, &divas_fops);
>>> 	if (!divas_proc_entry)
>>> 		return (0);
>