The attached patch abstracts out the namespace initialisation so that
temporary namespaces can be set up elsewhere.
Signed-Off-By: David Howells <[email protected]>
Acked-By: Sam Vilain <[email protected]>
---
David,
This looks sane to me, thought I'd just quickly ack it as I'm also doing
work in this area... it seems a lot of what you're doing is cleaning up
the boundaries between VFS and FS - did you get a chance to review the
patch Herbert P?tzl sent to the list about the permission() cleanup?
fs/namespace.c | 8 +-------
include/linux/namespace.h | 15 +++++++++++++++
2 files changed, 16 insertions(+), 7 deletions(-)
diff --git a/fs/namespace.c b/fs/namespace.c
index 51d3ebc..0194538 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -1688,13 +1688,7 @@ static void __init init_mount_tree(void)
namespace = kmalloc(sizeof(*namespace), GFP_KERNEL);
if (!namespace)
panic("Can't allocate initial namespace");
- atomic_set(&namespace->count, 1);
- INIT_LIST_HEAD(&namespace->list);
- init_waitqueue_head(&namespace->poll);
- namespace->event = 0;
- list_add(&mnt->mnt_list, &namespace->list);
- namespace->root = mnt;
- mnt->mnt_namespace = namespace;
+ init_namespace(namespace, mnt);
init_task.namespace = namespace;
read_lock(&tasklist_lock);
diff --git a/include/linux/namespace.h b/include/linux/namespace.h
index 3abc8e3..ea6fd62 100644
--- a/include/linux/namespace.h
+++ b/include/linux/namespace.h
@@ -17,6 +17,21 @@ extern int copy_namespace(int, struct ta
extern void __put_namespace(struct namespace *namespace);
extern struct namespace *dup_namespace(struct task_struct *, struct
fs_struct *);
+static inline void init_namespace(struct namespace *namespace,
+ struct vfsmount *mnt)
+{
+ atomic_set(&namespace->count, 1);
+ INIT_LIST_HEAD(&namespace->list);
+ init_waitqueue_head(&namespace->poll);
+ namespace->event = 0;
+ namespace->root = mnt;
+
+ if (mnt) {
+ list_add(&mnt->mnt_list, &namespace->list);
+ mnt->mnt_namespace = namespace;
+ }
+}
+
static inline void put_namespace(struct namespace *namespace)
{
if (atomic_dec_and_lock(&namespace->count, &vfsmount_lock))
On Thu, Mar 02, 2006 at 10:37:03AM +1300, Sam Vilain wrote:
> The attached patch abstracts out the namespace initialisation so that
> temporary namespaces can be set up elsewhere.
Definitily shouldb't be inline. And until you have a second caller
it's utterly pointless. So NACK for now.
Christoph Hellwig <[email protected]> wrote:
> > The attached patch abstracts out the namespace initialisation so that
> > temporary namespaces can be set up elsewhere.
>
> Definitily shouldb't be inline. And until you have a second caller
> it's utterly pointless. So NACK for now.
I presume from that you didn't notice that the second caller was in patch 5/5?
David
David Howells wrote:
> Christoph Hellwig <[email protected]> wrote:
>>>The attached patch abstracts out the namespace initialisation so that
>>>temporary namespaces can be set up elsewhere.
>>Definitily shouldb't be inline. And until you have a second caller
>>it's utterly pointless. So NACK for now.
> I presume from that you didn't notice that the second caller was in patch 5/5?
AIUI, each patch must stand on its own in every regard. I guess you
need to make it inline in the later patch - or not at all given the
marginal speed difference vs. core size increase.
Sam.
Sam Vilain wrote:
> The attached patch abstracts out the namespace initialisation so that
> temporary namespaces can be set up elsewhere.
>
> Signed-Off-By: David Howells <[email protected]>
> Acked-By: Sam Vilain <[email protected]>
Sorry, folks, forgot the From: line in my Acks. These patches are
David's work, not mine :)
Sam.
Sam Vilain <[email protected]> wrote:
> AIUI, each patch must stand on its own in every regard. I guess you
> need to make it inline in the later patch - or not at all given the
> marginal speed difference vs. core size increase.
No. It has to be permissable to make a series of patches that depend one upon
another for at least three reasons:
(1) Patches can be unmanageably large in one lump, so splitting them up is a
sensible option, even through the individual patches won't work or even
compile independently.
(2) It may make sense to place linked changes to two logically separate units
in two separate patches, for instance I'm changing the core kernel to add
an extra argument to get_sb() and the get_sb_*() convenience functions in
one patch and then supplying another patch to change all the filesystems.
This makes it much easier for a reviewer to see what's going on. They know
the patches are interdependent, but they can see the main core of the
changes separated out from the massively repetative but basically less
interesting changes that are a side effect of the main change.
(3) A series of patches may form a set of logical steps (for instance my
patches 1-2 are the first step and patches 3-5 the second). It may be (and
it is in my case) that each step will build and run, provided all the
previous steps are applied; but that a step won't build or run without the
preceding steps.
Remember: one of the main reasons for splitting patches is to make it easier
for other people to appreciate just how sublimely terrific your work is:-)
David
David Howells wrote:
>>AIUI, each patch must stand on its own in every regard. I guess you
>>need to make it inline in the later patch - or not at all given the
>>marginal speed difference vs. core size increase.
>>
>>
>
>No. It has to be permissable to make a series of patches that depend one upon
>another for at least three reasons:
>
> (1) Patches can be unmanageably large in one lump, so splitting them up is a
> sensible option, even through the individual patches won't work or even
> compile independently.
>
> (2) It may make sense to place linked changes to two logically separate units
> in two separate patches, for instance I'm changing the core kernel to add
> an extra argument to get_sb() and the get_sb_*() convenience functions in
> one patch and then supplying another patch to change all the filesystems.
>
> This makes it much easier for a reviewer to see what's going on. They know
> the patches are interdependent, but they can see the main core of the
> changes separated out from the massively repetative but basically less
> interesting changes that are a side effect of the main change.
>
> (3) A series of patches may form a set of logical steps (for instance my
> patches 1-2 are the first step and patches 3-5 the second). It may be (and
> it is in my case) that each step will build and run, provided all the
> previous steps are applied; but that a step won't build or run without the
> preceding steps.
>
>Remember: one of the main reasons for splitting patches is to make it easier
>for other people to appreciate just how sublimely terrific your work is:-)
>
>
Interesting. I've just seen patches slammed by subsystem maintainers
before for doing things "the wrong way around" within a patchset.
I don't remember seeing this covered in TPP, am I missing having read a
guide document or is this grey area?
Sam.
On Thu, Mar 02, 2006 at 09:12:23PM +0000, David Howells wrote:
> No. It has to be permissable to make a series of patches that depend one upon
> another for at least three reasons:
>
> (1) Patches can be unmanageably large in one lump, so splitting them up is a
> sensible option, even through the individual patches won't work or even
> compile independently.
That breaks git-bisect.
> (2) It may make sense to place linked changes to two logically separate units
> in two separate patches, for instance I'm changing the core kernel to add
> an extra argument to get_sb() and the get_sb_*() convenience functions in
> one patch and then supplying another patch to change all the filesystems.
>
> This makes it much easier for a reviewer to see what's going on. They know
> the patches are interdependent, but they can see the main core of the
> changes separated out from the massively repetative but basically less
> interesting changes that are a side effect of the main change.
It's also much easier to read a series of patches if each patch depends
only on the previous patches. Then if I want to verify that they don't
break anything, I just need to read them through in order and verify
that each one is correct.
If earlier patches depend on later patches, then I may not be able to
verify correctness until I've read and understand the whole series,
which defeats somewhat the purpose of splitting up the patches. Though
the above example wouldn't really be a problem.
And of course it seems rather silly to complain about splitting out a
function before adding the new caller.
--b.
Sam Vilain <[email protected]> wrote:
>
> >Remember: one of the main reasons for splitting patches is to make it easier
> >for other people to appreciate just how sublimely terrific your work is:-)
> >
> >
>
> Interesting. I've just seen patches slammed by subsystem maintainers
> before for doing things "the wrong way around" within a patchset.
>
> I don't remember seeing this covered in TPP, am I missing having read a
> guide document or is this grey area?
I just updated it.
--- tpp.txt 2006-03-04 16:32:28.000000000 -0800
+++ tpp2.txt 2006-03-04 16:33:10.000000000 -0800
@@ -1,7 +1,7 @@
The perfect patch.
[email protected]
-Updated 12 Jan 2006
+Updated 4 March 2006
The latest version of this document may be found at
http://www.zip.com.au/~akpm/linux/patches/stuff/tpp.txt
@@ -93,8 +93,8 @@
patch should contain a standalone changelog. This implies that you need a
patch management system which maintains changelogs. See below.
-e) Add a Signed-off-by: line, as per the Documentation/SubmittingPatches
- file in the kernel tree.
+e) Add a Signed-off-by: line, as per section 11 of the
+ Documentation/SubmittingPatches file in the kernel tree.
Signed-off-by: implies that you had some part in the developent of the
patch, or that you handled it and passed it on to another developer for
@@ -174,8 +174,49 @@
done
-6: Overall
-=========
+6: Patch series
+===============
+
+a) When sending a series of patches, number them in the Subject:s thusly:
+
+ [patch 1/10] ext2: block allocation: frob the globnozzle
+ [patch 2/10] ext2: block allocation: wash the pizza
+ etc
+
+b) Some people like to introduce a patch series with an introductory email
+ which doesn't actually carry a patch, such as:
+
+ [patch 0/10] ext2: block allocation changes
+
+ Please don't do this. There is no facility in the git tree to carry
+ changelog-only changesets such as this (or at least, we don't do that) so
+ the information in the introductory email will be lost.
+
+ So I end up copying and pasting your nice introduction into the
+ changelog for the first patch, so it gets into git. I'll follow it with
+ the text
+
+ This patch:
+
+ and then I'll include the changelog for the first patch of the series.
+
+ It would be preferred if the patch originators were to do this.
+
+c) Try very hard to ensure that the kernel builds and runs correctly at
+ every step of the patch series. This requirement exists because of
+ `git-bisect'. If someone is doing a bisection search for a kernel bug and
+ they land upon your won't-compile point partway through the exercise, they
+ will be unhappy.
+
+d) If your patch series includes non-runtime-affecting things such as
+ cleanups, whitespace fixes, file renames, moving functions around, etc then
+ this work should be done in the initial patches in the series. The
+ functional changes should come later in the series.
+
+ This is mainly so that reversion of problematic changes becomes simpler.
+
+7: Overall
+==========
a) Avoid MIME and attachements if possible. Make sure that your email
client does not wordwrap your patch. Make sure that your email client does