Ping: d_obtain_alias

Ping: d_obtain_alias

[Benny Halevy]

Christoph?

On Feb. 16, 2009, 19:31 +0200, Benny Halevy <bhalevy@panasas.com> wrote:

Christoph,

I see that in 4ea3ada2955e4519befa98ff55dd62d6dfbd1705,
you declared d_obtain_alias() as EXPORT_SYMBOL_GPL
and that it's supposed to replace d_alloc_anon which
was declared as EXPORT_SYMBOL and thus available to any
loadable module.

Do you consider d_obtain_alias() an internal API?
What exported API non-GPL'ed loadable file systems are
supposed to use now for fh_to_dentry?

Benny

Re: Ping: d_obtain_alias

[Benny Halevy]

On Feb. 21, 2009, 19:40 +0200, Christoph Hellwig <hch-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org> wrote:
> All the nfs exported filesystem are tied intimately to the kernel,
> so yes, this is an internal export.

I think that it would help if linux had a clear technical definition of
what being "intimately tied to" actually means and what makes usage
of an API derived work or not.

By making d_obtain_alias EXPORT_SYMBOL_GPL you made the exported API in
fs/dcache.c inconsistent.  What makes d_obtain_alias so different from
other functions in this file that are exported using EXPORT_SYMBOL?

looking at Documentation/DocBook/kernel-hacking.tmpl the guideline
it gives for using EXPORT_SYMBOL_GPL is "It implies that the function
is considered an internal implementation issue, and not really an
interface."

Rather than ranting about licensing I'd rather spend time on thinking
about what makes a function a part of a module's formal API
that's intended to be used by modules that are "consumers" or "users"
of the API (regardless of their license!) and what functions are exported
as a kernel-internal interface to other modules that co-operate with the
module (call them co-modules if you'd like) and that would be just
public (and not exported) if both modules were statically linked together.

I think that confusing licensing with the export symbol definition is
obstructing the technical part of the API design...

> 
> Non-GPL filesystem are illegal to distribute anyway, so I don't know why
> you even bother.

Hmm, I won't get down to this level of argument.

Benny

> 
> On Sat, Feb 21, 2009 at 09:13:21AM +0200, Benny Halevy wrote:
>> Christoph?
>>
>> On Feb. 16, 2009, 19:31 +0200, Benny Halevy <bhalevy-C4P08NqkoRlBDgjK7y7TUQ@public.gmane.org> wrote:
>>
>> Christoph,
>>
>> I see that in 4ea3ada2955e4519befa98ff55dd62d6dfbd1705,
>> you declared d_obtain_alias() as EXPORT_SYMBOL_GPL
>> and that it's supposed to replace d_alloc_anon which
>> was declared as EXPORT_SYMBOL and thus available to any
>> loadable module.
>>
>> Do you consider d_obtain_alias() an internal API?
>> What exported API non-GPL'ed loadable file systems are
>> supposed to use now for fh_to_dentry?
>>
>> Benny
> ---end quoted text---
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Ping: d_obtain_alias

[Benny Halevy]

On Feb. 21, 2009, 19:40 +0200, Christoph Hellwig <hch@infradead.org> wrote:
> All the nfs exported filesystem are tied intimately to the kernel,
> so yes, this is an internal export.

I think that it would help if linux had a clear technical definition of
what being "intimately tied to" actually means and what makes usage
of an API derived work or not.

By making d_obtain_alias EXPORT_SYMBOL_GPL you made the exported API in
fs/dcache.c inconsistent.  What makes d_obtain_alias so different from
other functions in this file that are exported using EXPORT_SYMBOL?

looking at Documentation/DocBook/kernel-hacking.tmpl the guideline
it gives for using EXPORT_SYMBOL_GPL is "It implies that the function
is considered an internal implementation issue, and not really an
interface."

Rather than ranting about licensing I'd rather spend time on thinking
about what makes a function a part of a module's formal API
that's intended to be used by modules that are "consumers" or "users"
of the API (regardless of their license!) and what functions are exported
as a kernel-internal interface to other modules that co-operate with the
module (call them co-modules if you'd like) and that would be just
public (and not exported) if both modules were statically linked together.

I think that confusing licensing with the export symbol definition is
obstructing the technical part of the API design...

> 
> Non-GPL filesystem are illegal to distribute anyway, so I don't know why
> you even bother.

Hmm, I won't get down to this level of argument.

Benny

> 
> On Sat, Feb 21, 2009 at 09:13:21AM +0200, Benny Halevy wrote:
>> Christoph?
>>
>> On Feb. 16, 2009, 19:31 +0200, Benny Halevy <bhalevy@panasas.com> wrote:
>>
>> Christoph,
>>
>> I see that in 4ea3ada2955e4519befa98ff55dd62d6dfbd1705,
>> you declared d_obtain_alias() as EXPORT_SYMBOL_GPL
>> and that it's supposed to replace d_alloc_anon which
>> was declared as EXPORT_SYMBOL and thus available to any
>> loadable module.
>>
>> Do you consider d_obtain_alias() an internal API?
>> What exported API non-GPL'ed loadable file systems are
>> supposed to use now for fh_to_dentry?
>>
>> Benny
> ---end quoted text---

Re: Ping: d_obtain_alias

[Christoph Hellwig]

All the nfs exported filesystem are tied intimately to the kernel,
so yes, this is an internal export.

Non-GPL filesystem are illegal to distribute anyway, so I don't know why
you even bother.

On Sat, Feb 21, 2009 at 09:13:21AM +0200, Benny Halevy wrote:
> Christoph?
> 
> On Feb. 16, 2009, 19:31 +0200, Benny Halevy <bhalevy@panasas.com> wrote:
> 
> Christoph,
> 
> I see that in 4ea3ada2955e4519befa98ff55dd62d6dfbd1705,
> you declared d_obtain_alias() as EXPORT_SYMBOL_GPL
> and that it's supposed to replace d_alloc_anon which
> was declared as EXPORT_SYMBOL and thus available to any
> loadable module.
> 
> Do you consider d_obtain_alias() an internal API?
> What exported API non-GPL'ed loadable file systems are
> supposed to use now for fh_to_dentry?
> 
> Benny
---end quoted text---

Re: Ping: d_obtain_alias

[Andrew Morton]

On Sat, 21 Feb 2009 12:40:46 -0500 Christoph Hellwig <hch-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org> wrote:
>

(top-posting repaired)

> On Sat, Feb 21, 2009 at 09:13:21AM +0200, Benny Halevy wrote:
> > Christoph?
> > 
> > On Feb. 16, 2009, 19:31 +0200, Benny Halevy <bhalevy-C4P08NqkoRlBDgjK7y7TUQ@public.gmane.org> wrote:
> > 
> > Christoph,
> > 
> > I see that in 4ea3ada2955e4519befa98ff55dd62d6dfbd1705,
> > you declared d_obtain_alias() as EXPORT_SYMBOL_GPL
> > and that it's supposed to replace d_alloc_anon which
> > was declared as EXPORT_SYMBOL and thus available to any
> > loadable module.
> > 
> > Do you consider d_obtain_alias() an internal API?
> > What exported API non-GPL'ed loadable file systems are
> > supposed to use now for fh_to_dentry?
> > 
> > Benny
> 
> All the nfs exported filesystem are tied intimately to the kernel,
> so yes, this is an internal export.
> 
> Non-GPL filesystem are illegal to distribute anyway, so I don't know why
> you even bother.
> 

Here's the patch in question:

commit 4ea3ada2955e4519befa98ff55dd62d6dfbd1705
Author: Christoph Hellwig <hch-jcswGhMUV9g@public.gmane.org>
Date:   Mon Aug 11 15:48:57 2008 +0200

    [PATCH] new helper: d_obtain_alias
    
    The calling conventions of d_alloc_anon are rather unfortunate for all
    users, and it's name is not very descriptive either.
    
    Add d_obtain_alias as a new exported helper that drops the inode
    reference in the failure case, too and allows to pass-through NULL
    pointers and inodes to allow for tail-calls in the export operations.
    
    Incidentally this helper already existed as a private function in
    libfs.c as exportfs_d_alloc so kill that one and switch the callers
    to d_obtain_alias.
    


Neither the title nor the changelog mentioned anything about disabling
non-GPL filesystems, so this effect was a mistake.  I didn't know this
was the effect and I doubt if Linus knew and quite possibly Christoph
and Al didn't know either.

We should correct that mistake.  Afterwards, feel free to prepare and
send out a patch titled "[patch] disable non-GPL filesystems" and we
can discuss it.

And before you start: I personally won't have any particularly strong
opinions in that discussion and will be interested to see the
discussion.

But preempting that discussion by leaving this mistake in place is not
the right way to go about this.

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Ping: d_obtain_alias

[Andrew Morton]

On Sat, 21 Feb 2009 12:40:46 -0500 Christoph Hellwig <hch@infradead.org> wrote:
>

(top-posting repaired)

> On Sat, Feb 21, 2009 at 09:13:21AM +0200, Benny Halevy wrote:
> > Christoph?
> > 
> > On Feb. 16, 2009, 19:31 +0200, Benny Halevy <bhalevy@panasas.com> wrote:
> > 
> > Christoph,
> > 
> > I see that in 4ea3ada2955e4519befa98ff55dd62d6dfbd1705,
> > you declared d_obtain_alias() as EXPORT_SYMBOL_GPL
> > and that it's supposed to replace d_alloc_anon which
> > was declared as EXPORT_SYMBOL and thus available to any
> > loadable module.
> > 
> > Do you consider d_obtain_alias() an internal API?
> > What exported API non-GPL'ed loadable file systems are
> > supposed to use now for fh_to_dentry?
> > 
> > Benny
> 
> All the nfs exported filesystem are tied intimately to the kernel,
> so yes, this is an internal export.
> 
> Non-GPL filesystem are illegal to distribute anyway, so I don't know why
> you even bother.
> 

Here's the patch in question:

commit 4ea3ada2955e4519befa98ff55dd62d6dfbd1705
Author: Christoph Hellwig <hch@lst.de>
Date:   Mon Aug 11 15:48:57 2008 +0200

    [PATCH] new helper: d_obtain_alias
    
    The calling conventions of d_alloc_anon are rather unfortunate for all
    users, and it's name is not very descriptive either.
    
    Add d_obtain_alias as a new exported helper that drops the inode
    reference in the failure case, too and allows to pass-through NULL
    pointers and inodes to allow for tail-calls in the export operations.
    
    Incidentally this helper already existed as a private function in
    libfs.c as exportfs_d_alloc so kill that one and switch the callers
    to d_obtain_alias.
    


Neither the title nor the changelog mentioned anything about disabling
non-GPL filesystems, so this effect was a mistake.  I didn't know this
was the effect and I doubt if Linus knew and quite possibly Christoph
and Al didn't know either.

We should correct that mistake.  Afterwards, feel free to prepare and
send out a patch titled "[patch] disable non-GPL filesystems" and we
can discuss it.

And before you start: I personally won't have any particularly strong
opinions in that discussion and will be interested to see the
discussion.

But preempting that discussion by leaving this mistake in place is not
the right way to go about this.

Re: Ping: d_obtain_alias

[Linus Torvalds]

On Sat, 21 Feb 2009, Andrew Morton wrote:
> 
> Here's the patch in question:

.. or perhaps 9308a6128d9074e348d9f9b5822546fe12a794a9, which actually did 
the "Remove d_alloc_anon now that no users are left.".

I do agree - if people are now expected to convert from d_alloc_anon() to 
d_obtain_alias(), then we can't just go and change license requirements 
under them. 

Some religious argument doesn't change that, or override technical 
reasoning. If people are still using d_alloc_anon(), we should either 
re-export it, or just export d_obtain_alias() in a usable form for them.

		Linus

[PATCH] EXPORT_SYMBOL(d_obtain_alias) rather than EXPORT_SYMBOL_GPL

[Benny Halevy]

Commit 4ea3ada2955e4519befa98ff55dd62d6dfbd1705
declares d_obtain_alias() as EXPORT_SYMBOL_GPL
where it's supposed to replace d_alloc_anon which
was previously declared as EXPORT_SYMBOL and thus
available to any loadable module.

This patch reverts that, as discussed below:

On Feb. 21, 2009, 11:40 -0800, Andrew Morton <akpm@linux-foundation.org> wrote:
> On Sat, 21 Feb 2009 12:40:46 -0500 Christoph Hellwig <hch@infradead.org> wrote:
...
>> All the nfs exported filesystem are tied intimately to the kernel,
>> so yes, this is an internal export.
>>
>> Non-GPL filesystem are illegal to distribute anyway, so I don't know why
>> you even bother.
>>
>
> Here's the patch in question:
...
> Neither the title nor the changelog mentioned anything about disabling
> non-GPL filesystems, so this effect was a mistake.  I didn't know this
> was the effect and I doubt if Linus knew and quite possibly Christoph
> and Al didn't know either.
>
> We should correct that mistake.  Afterwards, feel free to prepare and
> send out a patch titled "[patch] disable non-GPL filesystems" and we
> can discuss it.

On Feb. 21, 2009, 11:49 -0800, Linus Torvalds <torvalds@linux-foundation.org> wrote:
>
> On Sat, 21 Feb 2009, Andrew Morton wrote:
>> Here's the patch in question:
>
> .. or perhaps 9308a6128d9074e348d9f9b5822546fe12a794a9, which actually did
> the "Remove d_alloc_anon now that no users are left.".
>
> I do agree - if people are now expected to convert from d_alloc_anon() to
> d_obtain_alias(), then we can't just go and change license requirements
> under them.
>
> Some religious argument doesn't change that, or override technical
> reasoning. If people are still using d_alloc_anon(), we should either
> re-export it, or just export d_obtain_alias() in a usable form for them.
>
> 		Linus

Signed-off-by: Benny Halevy <bhalevy@panasas.com>
---
 fs/dcache.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/fs/dcache.c b/fs/dcache.c
index 937df0f..07e2d4a 100644
--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -1180,7 +1180,7 @@ struct dentry *d_obtain_alias(struct inode *inode)
 	iput(inode);
 	return res;
 }
-EXPORT_SYMBOL_GPL(d_obtain_alias);
+EXPORT_SYMBOL(d_obtain_alias);
 
 /**
  * d_splice_alias - splice a disconnected dentry into the tree if one exists
-- 
1.6.1.3

Re: [PATCH] EXPORT_SYMBOL(d_obtain_alias) rather than EXPORT_SYMBOL_GPL

[Al Viro]

On Tue, Feb 24, 2009 at 12:11:42PM -0800, Benny Halevy wrote:
> Commit 4ea3ada2955e4519befa98ff55dd62d6dfbd1705
> declares d_obtain_alias() as EXPORT_SYMBOL_GPL
> where it's supposed to replace d_alloc_anon which
> was previously declared as EXPORT_SYMBOL and thus
> available to any loadable module.
> 
> This patch reverts that, as discussed below:

ACK.  d_obtain_alias() makes sense as a general-purpose interface, period.

	As an aside, EXPORT_SYMBOL_GPL() is obnoxious shite anyway.  If
something is not a safe API, exporting it to default-quality 3rd-party
code is bad and while GPL might be many things, it is not a magical
"teach average programmer to find his arse without a map" pill.

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] EXPORT_SYMBOL(d_obtain_alias) rather than EXPORT_SYMBOL_GPL

[Al Viro]

On Tue, Feb 24, 2009 at 12:11:42PM -0800, Benny Halevy wrote:
> Commit 4ea3ada2955e4519befa98ff55dd62d6dfbd1705
> declares d_obtain_alias() as EXPORT_SYMBOL_GPL
> where it's supposed to replace d_alloc_anon which
> was previously declared as EXPORT_SYMBOL and thus
> available to any loadable module.
> 
> This patch reverts that, as discussed below:

ACK.  d_obtain_alias() makes sense as a general-purpose interface, period.

	As an aside, EXPORT_SYMBOL_GPL() is obnoxious shite anyway.  If
something is not a safe API, exporting it to default-quality 3rd-party
code is bad and while GPL might be many things, it is not a magical
"teach average programmer to find his arse without a map" pill.

Re: [PATCH] EXPORT_SYMBOL(d_obtain_alias) rather than EXPORT_SYMBOL_GPL

[Linus Torvalds]

On Tue, 24 Feb 2009, Al Viro wrote:
> 
> As an aside, EXPORT_SYMBOL_GPL() is obnoxious shite anyway.  If
> something is not a safe API, exporting it to default-quality 3rd-party
> code is bad and while GPL might be many things, it is not a magical
> "teach average programmer to find his arse without a map" pill.

Actually, I have had lawyers tell me it's actually a good idea, and shows 
"intent".  Intent may not matter for code in the sense that the code does 
what the code does regardless of what the programmer _intended_ it to do, 
but it does matter in legal terms.

So don't think of it in technical terms. It's not about good quality or 
about finding your arse without maps.

			Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] EXPORT_SYMBOL(d_obtain_alias) rather than EXPORT_SYMBOL_GPL

[Linus Torvalds]

On Tue, 24 Feb 2009, Al Viro wrote:
> 
> As an aside, EXPORT_SYMBOL_GPL() is obnoxious shite anyway.  If
> something is not a safe API, exporting it to default-quality 3rd-party
> code is bad and while GPL might be many things, it is not a magical
> "teach average programmer to find his arse without a map" pill.

Actually, I have had lawyers tell me it's actually a good idea, and shows 
"intent".  Intent may not matter for code in the sense that the code does 
what the code does regardless of what the programmer _intended_ it to do, 
but it does matter in legal terms.

So don't think of it in technical terms. It's not about good quality or 
about finding your arse without maps.

			Linus