Messages in this thread | | | From | borsa77@libero ... | Date | Wed, 21 Dec 2005 01:15:52 +0100 | Subject | Re: [PATCH] Correction to kmod.c control loop |
| |
On 21 Dec 2005 at 0:11, Jesper Juhl wrote:
> > That is the right way of course, but the portion of kmod.c I look is the > > same from 2.2.x series. > > > That may be so, but if you don't work against the latest kernel your > patches are unlikely to apply cleanly (other nearby bits of the file > may have changed causing patch to become confused), and also, > maintainers will usually request that you submit a re-diff'ed patch > against the latest kernel *anyway* before they will consider merging > your patch, so by working against an old kernel you are only creating > more work for both yourself and people reviewing and/or considering > applying your patches. > Work to make review and merging as easy on everyone else as you can - > starting out by submitting patches only against latest 2.4.x or 2.6.x > is a good place to start.
I look in the kmod.c for 2.6.x that you have linked below, actually it was reimplemented, so the patch is only for 2.4.x.
> > > Why are you changing the type of waitpid_result ? > > > > Because the man page for waitpid function tells the return type is pid_t. > > > Ok, but you just created a potential problem by then later returning > pid_t from a function supposed to be returning int. > When making changes like this you should explain in your mail that go > along with your patch *why* you are changing things and how you've > made sure they are safe - you need to be able to explain that.
It would be only a formal change, and as you have reported below pid_t is an integer.
> > > You changed MAX_KMOD_CONCURRENT from a constant to a variable above, > > > but you never assign a value to it, so here you are comparing i to an > > > uninitialized variable, not good. > > > > > > It is a _static_ local variable so it is assigned automatically to zero, I > > think. > > > Not good enough. Either you *know* that it will be initialized to > zero, and if so you should state that in your explanation of what the > patch does or you should explicitly initialize it.
I have read the K&R: now I know.
> > > > if (atomic_read(&kmod_concurrent) > i) { > > > > @@ -208,6 +206,7 @@ > > > > printk(KERN_ERR > > > > "kmod: runaway modprobe > > loop assumed > > > > and stopped\n"); > > > > atomic_dec(&kmod_concurrent); > > > > + MAX_KMOD_CONCURRENT = > > > > 2*MAX_KMOD_CONCURRENT+1; > > > > > > why multiply by two and add 1 here? > > > > > > For to grow up the previously zero value at each failure loop. > > See (ii) below. > > > Yes, I understand that you wish to grow it, it's just not clear to me > why you want to grow it like that.
To tune system capability to user request.
> > > > return -ENOMEM; > > > > } > > > > > > > > @@ -237,6 +236,7 @@ > > > > if (waitpid_result != pid) { > > > > printk(KERN_ERR "request_module[%s]: waitpid(%d,...) > > > > failed, errno %d\n", > > > > module_name, pid, -waitpid_result); > > > > + return waitpid_result; > > > > > > Ehh, the function returns an int, but you just changed the type of > > > waitpid_result to pid_t above... > > > > > > True, I had better control with grep that pid_t is an integer type. > > > pid_t is defined as > typedef __kernel_pid_t pid_t; > and __kernel_pid_t is > typedef int __kernel_pid_t; > on all archs as far as I know, so you end up actually being safe, but > the problem is you didn't make that clear. > But, the point I was trying to make was mainly that you shouldn't > count on that always being true. if the function needs to return an > int you should return an int, not a pid_t. > > Also, take a look at how the function is implemented in recent kernels : > http://sosdg.org/~coywolf/lxr/source/kernel/kmod.c#L66 > > > > > > > } > > > > return 0; > > > > }
- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |