Re: om-2.6 patch for kcomd (pre-pre-alpha)

Welcome back Tab,
  A month with no mentor to guide me on the coding was long enough. :)

On Wed, 2006-07-19 at 17:05 +0000, Vincent Hanquez  wrote:
> On Fri, Jul 14, 2006 at 01:36:15PM -0600, Matt Dew wrote:
> > DOH! Accidentally hit send.  Sorry 'bout that.
> > 
> > I've put my stuff on 
> > http://www.osource.org/openmosix/om-26.html
> > 
> > The patch is against the vanilla 2.6.16 instead of against git.
> > 
> > I've tested the patch on there now, om-2.6-full-20060714.patch.bz2,
and
> > it applies, compiles and runs.
> 
> Is that possible to have a patch against latest git ?
http://www.osource.org/openmosix/patches/om-2.6-git-20060714.patch.bz2

> just some random bit of my quick review:
> 
> - do not continue after accept_connection
Why not?  Finishing the loop gets us nothing.


> - spin_lock and unlock really need to be there when browsing
>   (list_for_each) the nodes. why has that been uncommented ?
I was having a weird problem with a lock not being released but I
couldn't tell where, so I finally just commented them all out so I could
get a patch out for people to look at.

> - some word about the CLONE_FS|CLOSE_FILES ? I believe that useful to
be
>   able to close all stuff from the module exit call. However that
seems
>   wrong .. not sure with which thread is that sharing fs/files.

I was trying to call sock_alloc_fd from under kcom_send_with* and have
kcomd use those file descriptors.   After some digging that's been
disabled elsewhere in the kernel for security purposes.  I didn't go
back and take out the kernel_thread flags.

> - header of kcom.h is wrong, please take the one from hpc/kcom.c
Noted.     Copyright Matt Dew.  ;)

> - you are still allocating the socket
I am?  I took out the kzalloc() and just left the sock_alloc()

> - lots of commented lines. that should definitively be cleaned up.
that's a given. :)

> - proc_pid_set_where looks overly changed, and most of the stuff
should
For debugging purposes. Easy place to put that stuff and to remove
later.  I've removed it as now I'm working on migrations.

>   not be there.
> - "char buf='testbug';" is that really compiling ?
yep.

> - p->om.whereto
> - too much slabs. I think only the pkt slab is needed.
So you're not worried about the kzallocs() for task, node, or whereto
assignments?

if nothing else, kcom_tasks also seems candidate for slabs.

> 
> With smaller documented patches, I may be able to be more specific
though.
Tell me where/how you want it broken up.

> 
> Cheers,
Matt


-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys -- and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV