[PATCH] misc: always assign miscdevice to file->private_data in open()

[PATCH] misc: always assign miscdevice to file->private_data in open()

[Martin Kepplinger]

As of now, a miscdevice driver has to provide an implementation of
the open() file operation if it wants to have misc_open() assign a
pointer to struct miscdevice to file->private_data for other file
operations to use (given the user calls open()).

This leads to situations where a miscdevice driver that doesn't need
internal operations during open() has to implement open() that only
returns immediately, in order to use the data in private_data in other
fops.

This change provides consistent behaviour for miscdevice developers by
always providing the pointer in private_data. A driver's open() fop would,
of course, just overwrite it, when using private_data itself.

Signed-off-by: Martin Kepplinger <martink@posteo.de>
---
This is really only a question: Do I understand this correctly, and,
could this change then hurt any existing driver?
As a driver developer it took me a while to figure out what happens here,
and in my situation it would have been nice to just have this feature as
part of the miscdevice API. Possibly documented somewhere?

misc_open() is called in any case, on open(). As long as miscdevice drivers
don't explicitly rely on private_data being NULL exactly IF they don't
implement an open() fop (which I wouldn't imagine), this would make things
even more convenient.

 drivers/char/misc.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/char/misc.c b/drivers/char/misc.c
index ffa97d2..205ad4c 100644
--- a/drivers/char/misc.c
+++ b/drivers/char/misc.c
@@ -142,8 +142,8 @@ static int misc_open(struct inode * inode, struct file * file)
 
 	err = 0;
 	replace_fops(file, new_fops);
+	file->private_data = c;
 	if (file->f_op->open) {
-		file->private_data = c;
 		err = file->f_op->open(inode,file);
 	}
 fail:
-- 
1.7.10.4

Re: [PATCH] misc: always assign miscdevice to file->private_data in open()

[Greg KH]

On Wed, Oct 08, 2014 at 10:47:54AM +0200, Martin Kepplinger wrote:
> As of now, a miscdevice driver has to provide an implementation of
> the open() file operation if it wants to have misc_open() assign a
> pointer to struct miscdevice to file->private_data for other file
> operations to use (given the user calls open()).
> 
> This leads to situations where a miscdevice driver that doesn't need
> internal operations during open() has to implement open() that only
> returns immediately, in order to use the data in private_data in other
> fops.

Yeah, that's messy, do we have any in-kernel misc drivers that do this?

> This change provides consistent behaviour for miscdevice developers by
> always providing the pointer in private_data. A driver's open() fop would,
> of course, just overwrite it, when using private_data itself.
> 
> Signed-off-by: Martin Kepplinger <martink@posteo.de>
> ---
> This is really only a question: Do I understand this correctly, and,
> could this change then hurt any existing driver?

I don't know, take a look at the existing ones and see please.

> As a driver developer it took me a while to figure out what happens here,
> and in my situation it would have been nice to just have this feature as
> part of the miscdevice API. Possibly documented somewhere?

Patches always accepted for documentation :)

> misc_open() is called in any case, on open(). As long as miscdevice drivers
> don't explicitly rely on private_data being NULL exactly IF they don't
> implement an open() fop (which I wouldn't imagine), this would make things
> even more convenient.

I agree, but it would be great if you can audit the existing misc
drivers to ensure we don't break anything with this change.  Can you do
that please?

thanks,

greg k-h

Re: [PATCH] misc: always assign miscdevice to file->private_data in open()

[Martin Kepplinger]

Am 2014-10-08 15:43, schrieb Greg KH:
> On Wed, Oct 08, 2014 at 10:47:54AM +0200, Martin Kepplinger wrote:
>> As of now, a miscdevice driver has to provide an implementation of
>> the open() file operation if it wants to have misc_open() assign a
>> pointer to struct miscdevice to file->private_data for other file
>> operations to use (given the user calls open()).
>>
>> This leads to situations where a miscdevice driver that doesn't need
>> internal operations during open() has to implement open() that only
>> returns immediately, in order to use the data in private_data in other
>> fops.
> 
> Yeah, that's messy, do we have any in-kernel misc drivers that do this?
> 
>> This change provides consistent behaviour for miscdevice developers by
>> always providing the pointer in private_data. A driver's open() fop would,
>> of course, just overwrite it, when using private_data itself.
>>
>> Signed-off-by: Martin Kepplinger <martink@posteo.de>
>> ---
>> This is really only a question: Do I understand this correctly, and,
>> could this change then hurt any existing driver?
> 
> I don't know, take a look at the existing ones and see please.
> 
>> As a driver developer it took me a while to figure out what happens here,
>> and in my situation it would have been nice to just have this feature as
>> part of the miscdevice API. Possibly documented somewhere?
> 
> Patches always accepted for documentation :)

What would be a good place for this?
Documentation/driver-model/device.txt or
Documentation/filesystem/vfs.txt like so? I'm not sure.

>From facd10cfa7539755e960dec8cc009934200e68ec Mon Sep 17 00:00:00 2001
From: Martin Kepplinger <martink@posteo.de>
Date: Thu, 9 Oct 2014 14:54:28 +0200
Subject: [PATCH] documentation: misc_open sets private_data for driver's
 open()

Signed-off-by: Martin Kepplinger <martink@posteo.de>
---
 Documentation/filesystems/vfs.txt |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/Documentation/filesystems/vfs.txt
b/Documentation/filesystems/vfs.txt
index 61d65cc..06df9d9 100644
--- a/Documentation/filesystems/vfs.txt
+++ b/Documentation/filesystems/vfs.txt
@@ -869,7 +869,8 @@ otherwise noted.
 	done the way it is because it makes filesystems simpler to
 	implement. The open() method is a good place to initialize the
 	"private_data" member in the file structure if you want to point
-	to a device structure
+	to a device structure. In the case of "struct miscdevice", when
+	you implement open() this is done automatically.

   flush: called by the close(2) system call to flush a file

-- 
1.7.10.4


> 
>> misc_open() is called in any case, on open(). As long as miscdevice drivers
>> don't explicitly rely on private_data being NULL exactly IF they don't
>> implement an open() fop (which I wouldn't imagine), this would make things
>> even more convenient.
> 
> I agree, but it would be great if you can audit the existing misc
> drivers to ensure we don't break anything with this change.  Can you do
> that please?
> 

I would grep -r "struct miscdevice" ./drivers/; and look at struct
file_operations of these results, see how their open() looks like, and
where they assign something to private_data.

If you have an idea for a script that lists all relevant files for me,
please tell me.

I queue this up but can't tell at all when it actually gets scheduled in ;)

I guess some do this work on their own because they don't know that
misc_open() already does it for them. It would probably be too much to
check what drivers could then just drop their open(). Interesting though
;) But in the short term, I think the appended documentation would help.

                              martin

Re: [PATCH] misc: always assign miscdevice to file->private_data in open()

[Greg KH]

On Thu, Oct 09, 2014 at 03:10:21PM +0200, Martin Kepplinger wrote:
> Am 2014-10-08 15:43, schrieb Greg KH:
> > On Wed, Oct 08, 2014 at 10:47:54AM +0200, Martin Kepplinger wrote:
> >> As of now, a miscdevice driver has to provide an implementation of
> >> the open() file operation if it wants to have misc_open() assign a
> >> pointer to struct miscdevice to file->private_data for other file
> >> operations to use (given the user calls open()).
> >>
> >> This leads to situations where a miscdevice driver that doesn't need
> >> internal operations during open() has to implement open() that only
> >> returns immediately, in order to use the data in private_data in other
> >> fops.
> > 
> > Yeah, that's messy, do we have any in-kernel misc drivers that do this?
> > 
> >> This change provides consistent behaviour for miscdevice developers by
> >> always providing the pointer in private_data. A driver's open() fop would,
> >> of course, just overwrite it, when using private_data itself.
> >>
> >> Signed-off-by: Martin Kepplinger <martink@posteo.de>
> >> ---
> >> This is really only a question: Do I understand this correctly, and,
> >> could this change then hurt any existing driver?
> > 
> > I don't know, take a look at the existing ones and see please.
> > 
> >> As a driver developer it took me a while to figure out what happens here,
> >> and in my situation it would have been nice to just have this feature as
> >> part of the miscdevice API. Possibly documented somewhere?
> > 
> > Patches always accepted for documentation :)
> 
> What would be a good place for this?
> Documentation/driver-model/device.txt or
> Documentation/filesystem/vfs.txt like so? I'm not sure.

There's no documentation for misc devices?  If not, just put it in
kerneldoc format in the misc .c file.

> >From facd10cfa7539755e960dec8cc009934200e68ec Mon Sep 17 00:00:00 2001
> From: Martin Kepplinger <martink@posteo.de>
> Date: Thu, 9 Oct 2014 14:54:28 +0200
> Subject: [PATCH] documentation: misc_open sets private_data for driver's
>  open()
> 
> Signed-off-by: Martin Kepplinger <martink@posteo.de>
> ---
>  Documentation/filesystems/vfs.txt |    3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/filesystems/vfs.txt
> b/Documentation/filesystems/vfs.txt
> index 61d65cc..06df9d9 100644
> --- a/Documentation/filesystems/vfs.txt
> +++ b/Documentation/filesystems/vfs.txt
> @@ -869,7 +869,8 @@ otherwise noted.
>  	done the way it is because it makes filesystems simpler to
>  	implement. The open() method is a good place to initialize the
>  	"private_data" member in the file structure if you want to point
> -	to a device structure
> +	to a device structure. In the case of "struct miscdevice", when
> +	you implement open() this is done automatically.

No, no one will notice this in the vfs.txt file, and the vfs doesn't
care about misc devices.

> >> misc_open() is called in any case, on open(). As long as miscdevice drivers
> >> don't explicitly rely on private_data being NULL exactly IF they don't
> >> implement an open() fop (which I wouldn't imagine), this would make things
> >> even more convenient.
> > 
> > I agree, but it would be great if you can audit the existing misc
> > drivers to ensure we don't break anything with this change.  Can you do
> > that please?
> > 
> 
> I would grep -r "struct miscdevice" ./drivers/; and look at struct
> file_operations of these results, see how their open() looks like, and
> where they assign something to private_data.
> 
> If you have an idea for a script that lists all relevant files for me,
> please tell me.

You just came up with one there, that should be a good start.

good luck,

greg k-h

[PATCH] char: documentation: more useful information about misc device

[Martin Kepplinger]

This might prevent code duplication in the future.

Signed-off-by: Martin Kepplinger <martink@posteo.de>
---
This is a suggestion for a place to put this information. I think this
makes sense but there might be a more appropriate place elsewhere.

 drivers/char/misc.c |    4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/char/misc.c b/drivers/char/misc.c
index ffa97d2..6a3d15b 100644
--- a/drivers/char/misc.c
+++ b/drivers/char/misc.c
@@ -171,6 +171,10 @@ static const struct file_operations misc_fops = {
  *	The structure passed is linked into the kernel and may not be
  *	destroyed until it has been unregistered.
  *
+ *	If struct miscdevice's fops contain an implementation of open()
+ *	the struct file's private data will be a pointer back to
+ *	struct miscdevice.
+ *
  *	A zero is returned on success and a negative errno code for
  *	failure.
  */
-- 
1.7.10.4

Re: [PATCH] misc: always assign miscdevice to file->private_data in open()

[Martin Kepplinger]

Am 2014-10-09 17:50, schrieb Greg KH:
> On Thu, Oct 09, 2014 at 03:10:21PM +0200, Martin Kepplinger wrote:
>> Am 2014-10-08 15:43, schrieb Greg KH:
>>> On Wed, Oct 08, 2014 at 10:47:54AM +0200, Martin Kepplinger wrote:
>>>> As of now, a miscdevice driver has to provide an implementation of
>>>> the open() file operation if it wants to have misc_open() assign a
>>>> pointer to struct miscdevice to file->private_data for other file
>>>> operations to use (given the user calls open()).
>>>>
>>>> This leads to situations where a miscdevice driver that doesn't need
>>>> internal operations during open() has to implement open() that only
>>>> returns immediately, in order to use the data in private_data in other
>>>> fops.
>>>
>>> Yeah, that's messy, do we have any in-kernel misc drivers that do this?

yes, at least drivers/video/fbdev/pxa3xx-gcu.c , maybe more and I don't
know if others do the work theirselves.

An audit if changing to always-set-private_data breaks drivers should be
doable in a reasonable timeframe. I don't think there would be a
problem; it'd be good if others take a look aswell though.

>>>
>>>> This change provides consistent behaviour for miscdevice developers by
>>>> always providing the pointer in private_data. A driver's open() fop would,
>>>> of course, just overwrite it, when using private_data itself.
>>>>
>>>> Signed-off-by: Martin Kepplinger <martink@posteo.de>
>>>> ---
>>>> This is really only a question: Do I understand this correctly, and,
>>>> could this change then hurt any existing driver?
>>>
>>> I don't know, take a look at the existing ones and see please.
>>>
>>>> As a driver developer it took me a while to figure out what happens here,
>>>> and in my situation it would have been nice to just have this feature as
>>>> part of the miscdevice API. Possibly documented somewhere?
>>>
>>> Patches always accepted for documentation :)
>>
>> What would be a good place for this?
>> Documentation/driver-model/device.txt or
>> Documentation/filesystem/vfs.txt like so? I'm not sure.
> 
> There's no documentation for misc devices?  If not, just put it in
> kerneldoc format in the misc .c file.
> 
>> >From facd10cfa7539755e960dec8cc009934200e68ec Mon Sep 17 00:00:00 2001
>> From: Martin Kepplinger <martink@posteo.de>
>> Date: Thu, 9 Oct 2014 14:54:28 +0200
>> Subject: [PATCH] documentation: misc_open sets private_data for driver's
>>  open()
>>
>> Signed-off-by: Martin Kepplinger <martink@posteo.de>
>> ---
>>  Documentation/filesystems/vfs.txt |    3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/filesystems/vfs.txt
>> b/Documentation/filesystems/vfs.txt
>> index 61d65cc..06df9d9 100644
>> --- a/Documentation/filesystems/vfs.txt
>> +++ b/Documentation/filesystems/vfs.txt
>> @@ -869,7 +869,8 @@ otherwise noted.
>>  	done the way it is because it makes filesystems simpler to
>>  	implement. The open() method is a good place to initialize the
>>  	"private_data" member in the file structure if you want to point
>> -	to a device structure
>> +	to a device structure. In the case of "struct miscdevice", when
>> +	you implement open() this is done automatically.
> 
> No, no one will notice this in the vfs.txt file, and the vfs doesn't
> care about misc devices.
> 
>>>> misc_open() is called in any case, on open(). As long as miscdevice drivers
>>>> don't explicitly rely on private_data being NULL exactly IF they don't
>>>> implement an open() fop (which I wouldn't imagine), this would make things
>>>> even more convenient.
>>>
>>> I agree, but it would be great if you can audit the existing misc
>>> drivers to ensure we don't break anything with this change.  Can you do
>>> that please?
>>>
>>
>> I would grep -r "struct miscdevice" ./drivers/; and look at struct
>> file_operations of these results, see how their open() looks like, and
>> where they assign something to private_data.
>>
>> If you have an idea for a script that lists all relevant files for me,
>> please tell me.
> 
> You just came up with one there, that should be a good start.
> 
> good luck,
> 
> greg k-h
> 

Re: [PATCH] misc: always assign miscdevice to file->private_data in open()

[Martin Kepplinger]

Am 2014-10-09 17:50, schrieb Greg KH:
> On Thu, Oct 09, 2014 at 03:10:21PM +0200, Martin Kepplinger wrote:
>> Am 2014-10-08 15:43, schrieb Greg KH:
>>> On Wed, Oct 08, 2014 at 10:47:54AM +0200, Martin Kepplinger wrote:
>>>> As of now, a miscdevice driver has to provide an implementation of
>>>> the open() file operation if it wants to have misc_open() assign a
>>>> pointer to struct miscdevice to file->private_data for other file
>>>> operations to use (given the user calls open()).
>>>>
>>>> This leads to situations where a miscdevice driver that doesn't need
>>>> internal operations during open() has to implement open() that only
>>>> returns immediately, in order to use the data in private_data in other
>>>> fops.
>>>
>>> Yeah, that's messy, do we have any in-kernel misc drivers that do this?
>>>
>>>> This change provides consistent behaviour for miscdevice developers by
>>>> always providing the pointer in private_data. A driver's open() fop would,
>>>> of course, just overwrite it, when using private_data itself.
>>>>
>>>> Signed-off-by: Martin Kepplinger <martink@posteo.de>
>>>> ---
>>>> This is really only a question: Do I understand this correctly, and,
>>>> could this change then hurt any existing driver?
>>>
>>> I don't know, take a look at the existing ones and see please.
>>>
>>>> As a driver developer it took me a while to figure out what happens here,
>>>> and in my situation it would have been nice to just have this feature as
>>>> part of the miscdevice API. Possibly documented somewhere?
>>>
>>> Patches always accepted for documentation :)
>>
>> What would be a good place for this?
>> Documentation/driver-model/device.txt or
>> Documentation/filesystem/vfs.txt like so? I'm not sure.
> 
> There's no documentation for misc devices?  If not, just put it in
> kerneldoc format in the misc .c file.
> 
>> >From facd10cfa7539755e960dec8cc009934200e68ec Mon Sep 17 00:00:00 2001
>> From: Martin Kepplinger <martink@posteo.de>
>> Date: Thu, 9 Oct 2014 14:54:28 +0200
>> Subject: [PATCH] documentation: misc_open sets private_data for driver's
>>  open()
>>
>> Signed-off-by: Martin Kepplinger <martink@posteo.de>
>> ---
>>  Documentation/filesystems/vfs.txt |    3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/filesystems/vfs.txt
>> b/Documentation/filesystems/vfs.txt
>> index 61d65cc..06df9d9 100644
>> --- a/Documentation/filesystems/vfs.txt
>> +++ b/Documentation/filesystems/vfs.txt
>> @@ -869,7 +869,8 @@ otherwise noted.
>>  	done the way it is because it makes filesystems simpler to
>>  	implement. The open() method is a good place to initialize the
>>  	"private_data" member in the file structure if you want to point
>> -	to a device structure
>> +	to a device structure. In the case of "struct miscdevice", when
>> +	you implement open() this is done automatically.
> 
> No, no one will notice this in the vfs.txt file, and the vfs doesn't
> care about misc devices.
> 
>>>> misc_open() is called in any case, on open(). As long as miscdevice drivers
>>>> don't explicitly rely on private_data being NULL exactly IF they don't
>>>> implement an open() fop (which I wouldn't imagine), this would make things
>>>> even more convenient.
>>>
>>> I agree, but it would be great if you can audit the existing misc
>>> drivers to ensure we don't break anything with this change.  Can you do
>>> that please?
>>>

applying said change to misc_open() core and removing open() from
video/fbdev/pxa3xx-gcu.c generates these warnings that I, at the moment,
don't fully understand. Do you konw what happens here?

In file included from arch/x86/vdso/vdso2c.c:161:0:
arch/x86/vdso/vdso2c.c: In function ‘main’:
arch/x86/vdso/vdso2c.h:118:6: warning: assuming signed overflow does not
occur when assuming that (X + c) < X is always false [-Wstrict-overflow]
In file included from arch/x86/vdso/vdso2c.c:165:0:
arch/x86/vdso/vdso2c.h:118:6: warning: assuming signed overflow does not
occur when assuming that (X + c) < X is always false [-Wstrict-overflow]

>>
>> I would grep -r "struct miscdevice" ./drivers/; and look at struct
>> file_operations of these results, see how their open() looks like, and
>> where they assign something to private_data.
>>
>> If you have an idea for a script that lists all relevant files for me,
>> please tell me.
> 
> You just came up with one there, that should be a good start.
> 
> good luck,
> 
> greg k-h
> 

[PATCH 1/3] misc: always assign miscdevice to file->private_data in open()

[Martin Kepplinger]

As of now, a miscdevice driver has to provide an implementation of
the open() file operation if it wants to have misc_open() assign a
pointer to struct miscdevice to file->private_data for other file
operations to use (given the user calls open()).

This leads to situations where a miscdevice driver that doesn't need
internal operations during open() has to implement open() that only
returns immediately, in order to use the data in private_data in other
fops.

This provides consistent behaviour for miscdevice developers and will
always provide the pointer in private_data. A driver's open() fop would,
of course, just overwrite it, when using private_data itself.

Signed-off-by: Martin Kepplinger <martink@posteo.de>
---

The mentioned warning is appearently unrelated here, and happens on mainline
v3.17 awell -.- sorry for the confusion.

This applies to 3.17 and is a call for review and opinions. Especially on
the followup patches.


 drivers/char/misc.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/char/misc.c b/drivers/char/misc.c
index ffa97d2..205ad4c 100644
--- a/drivers/char/misc.c
+++ b/drivers/char/misc.c
@@ -142,8 +142,8 @@ static int misc_open(struct inode * inode, struct file * file)
 
 	err = 0;
 	replace_fops(file, new_fops);
+	file->private_data = c;
 	if (file->f_op->open) {
-		file->private_data = c;
 		err = file->f_op->open(inode,file);
 	}
 fail:
-- 
1.7.10.4

[PATCH 2/3] fbdev: pxa3xx-gcu: remove redundant implementation of open()

[Martin Kepplinger]

the miscdevice core now does the work in any case.

Signed-off-by: Martin Kepplinger <martink@posteo.de>
---
 drivers/video/fbdev/pxa3xx-gcu.c |   10 ----------
 1 file changed, 10 deletions(-)

diff --git a/drivers/video/fbdev/pxa3xx-gcu.c b/drivers/video/fbdev/pxa3xx-gcu.c
index 4df3657..7678a94 100644
--- a/drivers/video/fbdev/pxa3xx-gcu.c
+++ b/drivers/video/fbdev/pxa3xx-gcu.c
@@ -373,15 +373,6 @@ static inline struct pxa3xx_gcu_priv *to_pxa3xx_gcu_priv(struct file *file)
 	return container_of(dev, struct pxa3xx_gcu_priv, misc_dev);
 }
 
-/*
- * provide an empty .open callback, so the core sets file->private_data
- * for us.
- */
-static int pxa3xx_gcu_open(struct inode *inode, struct file *file)
-{
-	return 0;
-}
-
 static ssize_t
 pxa3xx_gcu_write(struct file *file, const char *buff,
 		 size_t count, loff_t *offp)
@@ -580,7 +571,6 @@ pxa3xx_gcu_free_buffers(struct device *dev,
 
 static const struct file_operations pxa3xx_gcu_miscdev_fops = {
 	.owner =		THIS_MODULE,
-	.open =			pxa3xx_gcu_open,
 	.write =		pxa3xx_gcu_write,
 	.unlocked_ioctl =	pxa3xx_gcu_ioctl,
 	.mmap =			pxa3xx_gcu_mmap,
-- 
1.7.10.4

[PATCH 3/3] lguest: force file->private_data to be NULL on open()

[Martin Kepplinger]

if we depend on private_data being NULL in write() before initialize()
make sure it is NULL after open().

Signed-off-by: Martin Kepplinger <martink@posteo.de>
---

I'm not completely sure if this patch is needed and am still investigating.
What do you think? open() could be called by the user I guess. Does
lguest_user.c depend on private_data being NULL on a first write()?


 drivers/lguest/lguest_user.c |    7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/lguest/lguest_user.c b/drivers/lguest/lguest_user.c
index 4263f4c..30251b7 100644
--- a/drivers/lguest/lguest_user.c
+++ b/drivers/lguest/lguest_user.c
@@ -497,6 +497,12 @@ static int close(struct inode *inode, struct file *file)
 	return 0;
 }
 
+static int open(struct inode *inode, struct file *file)
+{
+	/* the miscdevice core sets private_data on open() */
+	file->private_data = NULL;
+}
+
 /*L:000
  * Welcome to our journey through the Launcher!
  *
@@ -514,6 +520,7 @@ static int close(struct inode *inode, struct file *file)
  */
 static const struct file_operations lguest_fops = {
 	.owner	 = THIS_MODULE,
+	.open	 = open,
 	.release = close,
 	.write	 = write,
 	.read	 = read,
-- 
1.7.10.4

Re: [PATCH 3/3] lguest: force file->private_data to be NULL on open()

[Martin Kepplinger]

Am 2014-10-19 02:31, schrieb Martin Kepplinger:
> if we depend on private_data being NULL in write() before initialize()
> make sure it is NULL after open().
> 
> Signed-off-by: Martin Kepplinger <martink@posteo.de>
> ---
> 
> I'm not completely sure if this patch is needed and am still investigating.
> What do you think? open() could be called by the user I guess. Does
> lguest_user.c depend on private_data being NULL on a first write()?
> 
> 

Could it be that this patch is not needed indeed or did I ask not clear
enough here and caused a misunderstanding:


> Martin Kepplinger <martink@posteo.de> writes:
>> hi
>>
>> Just a question for understanding: open() is not implemented in
>> lguest_user.c's miscdevice. The miscdevice core, in this case, does
>> _not_ set file->private_data on a user's open() call. Is open() called
>> by the user here? and do you here _depend_ on file->private_data being
>> NULL after open()? (could you even?)
>>
>> Would the following force to NULL be necessary if the miscdevice core
>> _would_ set private_data?
>
> Hi Martin!
>
>         Yes, the private_data is NULL on a new file.  See
> get_empty_filp in fs/file_table.c, which does kmem_cache_zalloc
> (zeroing all the contents).
>
> So this isn't necessary here.
>
> Thanks!
> Rusty.