Re: [PATCH] btrfs: explicitly set control file's private_data

[David Sterba <dsterba@suse.cz>]

On Mon, Mar 23, 2015 at 06:34:13PM +0100, Tom Van Braeckel wrote:
> The private_data member of the Btrfs control device file
> (/dev/btrfs-control) is used to hold the current transaction and needs
> to be initialized to NULL to signify that no transaction is in progress.
> 
> We explicitly set the control file's private_data to NULL to be
> independent of whatever value the misc subsystem initializes it to.
> 
> Backstory:
> ----------
> 
> The misc subsystem (which is used by /dev/btrfs-control) initializes
> a file's private_data to point to the misc device when a driver has
> registered a custom open file operation and initializes it to NULL
> when a custom open file operation has *not* been provided.
> 
> This subtle quirk is confusing, to the point where kernel code registers
> *empty* file open operations to have private_data point to the misc
> device structure.
> 
> And it leads to bugs, where the addition or removal of a custom open
> file operation surprisingly changes the initial contents of a file's
> private_data structure.
> 
> To simplify things in the misc subsystem, a patch [1] has been proposed
> to *always* set private_data to point to the misc device instead of
> only doing this when a custom open file operation has been registered.
> 
> But before we can fix this in the misc subsystem itself, we need to
> modify the (few) drivers that rely on this very subtle behavior.
> 
> [1] https://lkml.org/lkml/2014/12/4/939
> 
> Signed-off-by: Martin Kepplinger <martink@posteo.de>
> Signed-off-by: Tom Van Braeckel <tomvanbraeckel@gmail.com>

Thanks for the explanation.

Acked-by: David Sterba <dsterba@suse.cz>

> +static int btrfs_control_open(struct inode *inode, struct file *file)
> +{
> +	/* The control file's private_data is used to hold the
> +	 * transaction when it is started and is used to keep
> +	 * track of whether a transaction is already in progress.
> +	 */

That's not the common comment style (newline after /* ) but I'm never
sure whether I should nitpick about such things or just say yes to
fixes.

> +	file->private_data = NULL;
> +	return 0;
> +}