[uml-devel] [patch 1/2] Uml - first part rework of run_helper() and users.

[<blaisorblade_spam@ya...>]

From: Jeff Dike <jdike@...>, Paolo 'Blaisorblade' Giarrusso <blaisorblade_spam@...>

Fixed a file descriptor leak in the network driver when changing an IP
address.

Fixed the error handling in run_helper.


Paolo notes:

Actually, this is part one of the change, the exact one extracted from Jeff
Dike's incrementals tree before 2.6.9-rc big UML merge.

There is some changes must be done, so I'm also sending a second patch with
this one, too. Separated for tracking purposes.

Don't send this pair of ones to Linus before Jeff ACK's it - just put into -mm for now.

Signed-off-by: Paolo 'Blaisorblade' Giarrusso <blaisorblade_spam@...>
---

 linux-2.6.10-rc-paolo/arch/um/drivers/net_user.c |    3 ++-
 linux-2.6.10-rc-paolo/arch/um/kernel/helper.c    |   14 +++++---------
 2 files changed, 7 insertions(+), 10 deletions(-)

diff -puN arch/um/drivers/net_user.c~uml-orig-helper-rework arch/um/drivers/net_user.c
--- linux-2.6.10-rc/arch/um/drivers/net_user.c~uml-orig-helper-rework	2004-11-30 20:43:45.701552240 +0100
+++ linux-2.6.10-rc-paolo/arch/um/drivers/net_user.c	2004-11-30 21:08:14.756221984 +0100
@@ -173,8 +173,9 @@ static int change_tramp(char **argv, cha
 	pe_data.stdout = fds[1];
 	pid = run_helper(change_pre_exec, &pe_data, argv, NULL);
 
-	os_close_file(fds[1]);
 	read_output(fds[0], output, output_len);
+	os_close_file(fds[0]);
+	os_close_file(fds[1]);
 
 	CATCH_EINTR(err = waitpid(pid, NULL, 0));
 	return(pid);
diff -puN arch/um/kernel/helper.c~uml-orig-helper-rework arch/um/kernel/helper.c
--- linux-2.6.10-rc/arch/um/kernel/helper.c~uml-orig-helper-rework	2004-11-30 20:43:45.703551936 +0100
+++ linux-2.6.10-rc-paolo/arch/um/kernel/helper.c	2004-11-30 21:08:14.755222136 +0100
@@ -94,24 +94,20 @@ int run_helper(void (*pre_exec)(void *),
 	if(n < 0){
 		printk("run_helper : read on pipe failed, err = %d\n", -n);
 		err = n;
-		goto out_kill;
+		os_kill_process(pid, 1);
 	}
 	else if(n != 0){
 		CATCH_EINTR(n = waitpid(pid, NULL, 0));
 		pid = -errno;
 	}
+	err = pid;
 
-	if(stack_out == NULL) free_stack(stack, 0);
-        else *stack_out = stack;
-	return(pid);
-
- out_kill:
-	os_kill_process(pid, 1);
  out_close:
 	os_close_file(fds[0]);
-	os_close_file(fds[1]);
  out_free:
-	free_stack(stack, 0);
+	if(stack_out == NULL)
+		free_stack(stack, 0);
+        else *stack_out = stack;
 	return(err);
 }
 
_

[uml-devel] Re: [patch 1/2] Uml - first part rework of run_helper() and users.

[Andrew Morton <akpm@os...>]

blaisorblade_spam@... wrote:
>
> Fixed a file descriptor leak in the network driver when changing an IP
> address.
> 
> Fixed the error handling in run_helper.
> 
> 
> Paolo notes:
> 
> Actually, this is part one of the change, the exact one extracted from Jeff
> Dike's incrementals tree before 2.6.9-rc big UML merge.
> 
> There is some changes must be done, so I'm also sending a second patch with
> this one, too. Separated for tracking purposes.
> 
> Don't send this pair of ones to Linus before Jeff ACK's it - just put into -mm for now.

That makes five UML patches which I have queued up pending confirmation:

hostfs-uml-set-sendfile-to-generic_file_sendfile.patch
hostfs-uml-add-some-other-pagecache-methods.patch
uml-terminal-cleanup.patch
uml-first-part-rework-of-run_helper-and-users.patch
uml-finish-fixing-run_helper-failure-path.patch

Could you gents please put heads together and tell me whether and when
these should go upstream?

Thanks.

[uml-devel] VFS interactions with UML and other big UML changes (was: Re: [patch 1/2] Uml - first part rework of run_helper() and users.)

[Blaisorblade <blaisorblade_spam@ya...>]

On Wednesday 01 December 2004 00:20, Andrew Morton wrote:
> blaisorblade_spam@... wrote:
> > Fixed a file descriptor leak in the network driver when changing an IP
> > address.
> >
> > Fixed the error handling in run_helper.

> > Paolo notes:
> >
> > Actually, this is part one of the change, the exact one extracted from
> > Jeff Dike's incrementals tree before 2.6.9-rc big UML merge.
> >
> > There is some changes must be done, so I'm also sending a second patch
> > with this one, too. Separated for tracking purposes.
> >
> > Don't send this pair of ones to Linus before Jeff ACK's it - just put
> > into -mm for now.

> That makes five UML patches which I have queued up pending confirmation:
Ok, detailed answers for each one.


> hostfs-uml-set-sendfile-to-generic_file_sendfile.patch
> hostfs-uml-add-some-other-pagecache-methods.patch

For these two, I'm waiting mainline answers - the first one was tested and 
worked, while the second not, but is no more intrusive. In general, the 
patches themselves are good: hostfs already makes full use of the page cache.

My doubt (which actually is not related to the patches themselves) is here:

static struct address_space_operations hostfs_aops = {
        .writepage      = hostfs_writepage,
        .readpage       = hostfs_readpage,
/*      .set_page_dirty = __set_page_dirty_nobuffers, */
        .prepare_write  = hostfs_prepare_write,
        .commit_write   = hostfs_commit_write
};

Actually, hostfs is a nodev filesystem, but I simply don't know if that 
implies that it uses no buffers. So, should

 .set_page_dirty = __set_page_dirty_nobuffers

be uncommented? Or should it be deleted (leaving it there is not a good 
option).

I'm holding the patches because I don't know if using them could anyhow 
trigger data loss from this bug, if it's a bug.

> uml-terminal-cleanup.patch

I don't know technically this one. It won't probably go in 2.6.10, I think 
later... tested in the SuSE tree, but let's be quiet in merging _big_ things, 
ok? It was also tested in a different tree, so it perfectly working on 2.6.9 
does not mean perfectly working on current kernels.

Some other well tested patches (not these ones) are causing host problems, 
i.e. UML processes crashing and staying in D state (this seems some kind of 
ptrace bug, but still digging on this) - acked on 2.6.9 hosts.

Or dying completely but keeping some FS (a tmpfs mount used only for UML) from 
being unmounted (yes, checked lsof, which shows nothing - I've heard rumors 
of locks alive). It does not seem to be related to tmpfs in particular, 
however.

> uml-first-part-rework-of-run_helper-and-users.patch
> uml-finish-fixing-run_helper-failure-path.patch
These are littler and somehow widely tested... but nobody complained with the 
1st alone.

> Could you gents please put heads together and tell me whether and when
> these should go upstream?
-- 
Paolo Giarrusso, aka Blaisorblade
Linux registered user n. 292729
http://www.user-mode-linux.org/~blaisorblade

[uml-devel] Re: VFS interactions with UML and other big UML changes (was: Re: [patch 1/2] Uml - first part rework of run_helper() and users.)

[Andrew Morton <akpm@os...>]

Blaisorblade <blaisorblade_spam@...> wrote:
>
> static struct address_space_operations hostfs_aops = {
>         .writepage      = hostfs_writepage,
>         .readpage       = hostfs_readpage,
> /*      .set_page_dirty = __set_page_dirty_nobuffers, */
>         .prepare_write  = hostfs_prepare_write,
>         .commit_write   = hostfs_commit_write
> };
> 
> Actually, hostfs is a nodev filesystem, but I simply don't know if that 
> implies that it uses no buffers. So, should
> 
>  .set_page_dirty = __set_page_dirty_nobuffers
> 
> be uncommented? Or should it be deleted (leaving it there is not a good 
> option).

See the operation of set_page_dirty().

If you have NULL ->set_page_dirty a_op then set_page_dirty() will fall
through to __set_page_dirty_buffers().

If your fs never sets PG_private then __set_page_dirty_buffers() will just
do what __set_page_dirty_nobuffers() does.

Without having looked at it, I'm sure that hostfs does not use
buffer_heads.  So setting your ->set_page_dirty a_op to point at
__set_page_dirty_nobuffers() is a reasonable thing to do - it'll provide a
slight speedup.

Re: [uml-devel] Re: VFS interactions with UML and other big UML changes (was: Re: [patch 1/2] Uml - first part rework of run_helper() and users.)

[Blaisorblade <blaisorblade_spam@ya...>]

On Wednesday 01 December 2004 01:33, Andrew Morton wrote:
> Blaisorblade <blaisorblade_spam@...> wrote:
> > static struct address_space_operations hostfs_aops = {
> >         .writepage      = hostfs_writepage,
> >         .readpage       = hostfs_readpage,
> > /*      .set_page_dirty = __set_page_dirty_nobuffers, */
> >         .prepare_write  = hostfs_prepare_write,
> >         .commit_write   = hostfs_commit_write
> > };
> >
> > Actually, hostfs is a nodev filesystem, but I simply don't know if that
> > implies that it uses no buffers. So, should
> >
> >  .set_page_dirty = __set_page_dirty_nobuffers
> >
> > be uncommented? Or should it be deleted (leaving it there is not a good
> > option).
>
> See the operation of set_page_dirty().

> If you have NULL ->set_page_dirty a_op then set_page_dirty() will fall
> through to __set_page_dirty_buffers().
Yes, I already understood this, the easy part.
> If your fs never sets PG_private then __set_page_dirty_buffers() will just
> do what __set_page_dirty_nobuffers() does.
Ok, I didn't imagine this (looks reasonable though).

Apart the fact that the "race with truncate" check is a bit different: this is 
is in __set_page_dirty_nobuffers(mm/page-writeback.c) and probably wants 
being added to the _buffers version, since it does cannot do anything else 
than triggering a BUG (which you don't see currently, I guess):

[...]
                        mapping2 = page_mapping(page);
                        if (mapping2) { /* Race with truncate? */
                                BUG_ON(mapping2 != mapping);
[...]

> Without having looked at it, I'm sure that hostfs does not use
> buffer_heads.

It can compile without 

#include <linux/buffer_head.h>

(even if the include is there), and it never seem to set any page as buffer 
(by setting the PG_private bit, which can have other meanings too I guess in 
other contexts).

So I guessed this right the first time - I was not sure if it was so 
straightforward.

> So setting your ->set_page_dirty a_op to point at 
> __set_page_dirty_nobuffers() is a reasonable thing to do - it'll provide a
> slight speedup.

If it is a speedup only, then I'm happier - I was especially worried if it was 
going to create possible bugs, even because there are someone has reported 
problems in listing large folders... never reproduced it here and most users 
don't see it, so not yet any clues.

Thanks a lot for the help!
-- 
Paolo Giarrusso, aka Blaisorblade
Linux registered user n. 292729
http://www.user-mode-linux.org/~blaisorblade

[uml-devel] Re: VFS interactions with UML and other big UML changes (was: Re: [patch 1/2] Uml - first part rework of run_helper() and users.)

[Gerd Knorr <kraxel@by...>]

> > uml-terminal-cleanup.patch
> 
> I don't know technically this one. It won't probably go in 2.6.10, I think 
> later... tested in the SuSE tree, but let's be quiet in merging _big_ things, 
> ok? It was also tested in a different tree, so it perfectly working on 2.6.9 
> does not mean perfectly working on current kernels.

Tested by me on 2.6.10-rc2-bk<something> as well.  It needed some
trivial adaptions to the tty layer changes done by Linus compared
to the old 2.6.9 version.  I'm pretty confident it wouldn't break
anything, but as it is to big to be classified as ObviouslyCorrect[tm]
fix it probably should not go to into 2.6.10 but be merged in the
2.6.11 cycle.

  Gerd

-- 
#define printk(args...) fprintf(stderr, ## args)

[uml-devel] Re: [patch 1/2] Uml - first part rework of run_helper() and users.

[Jeff Dike <jdike@ad...>]

akpm@... said:
> That makes five UML patches which I have queued up pending
> confirmation:
>
> hostfs-uml-set-sendfile-to-generic_file_sendfile.patch
> hostfs-uml-add-some-other-pagecache-methods.patch 
> uml-terminal-cleanup.patch 
> uml-first-part-rework-of-run_helper-and-users.patch
> uml-finish-fixing-run_helper-failure-path.patch 

These are all fine.  Feel free to send them to Linus.

				Jeff