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
--

[ message continues ]

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.

---end quoted text---
--

[ message continues ]

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

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

--

[ message continues ]

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.

--

[ message continues ]

.. 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
--

[ message continues ]

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:



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


--

[ message continues ]

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.

--

[ message continues ]

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
--

[ message continues ]