Re: [patch] openmosix/openmosix-documentation.patch

Sigh, it's almost funny. that now that I have no longer any time to work
on the project is when it starts to move again.  ;)

On Mon, 2006-10-02 at 17:07 +0000, Vincent Hanquez wrote: 
> On Mon, Oct 02, 2006 at 06:56:16PM +0200, Florian Delizy wrote:
> >  /**
> > - * deputy_copy_from_user - Copy from remote when running on deputy
> > + * deputy_copy_from_user -
> > + * @to:     kernelspace address to copy to
> > + * @from:   userspace address to copy from
> > + * @n:      size of data to copy
> > + *
> > + * Description:
> > + *    Copy from remote when running on deputy
> >   **/
> 
> why is that moving the short description into the long description field ?
> specially since the text is the same.

Consistency.

> 
> > [snip]
> >  /**
> > - * deputy_put_userX - put a value of 64 bit or less to remote
> > + * deputy_put_userX
> > + * @value:
> > + * @addr:
> > + * @size:
> > + *
> > + * Description:
> > + *    put a value of 64 bit or less to remote
> >   **/
> 
> (same move from short description to long description ..)
> why to document a function parameter if you don't put the documentation
> that come with it ???

Feel free to add the proper descriptions.

> 
> > [snip]
> > Index: linux/hpc/kcomd.c
> > ===================================================================
> > --- linux.orig/hpc/kcomd.c	2006-09-28 14:53:08.000000000 +0200
> > +++ linux/hpc/kcomd.c	2006-09-28 14:53:30.000000000 +0200
> > @@ -21,11 +21,17 @@
> >  #include <net/sock.h>
> >  #include <net/tcp.h>
> >  
> > +/**
> > + * socket_listen
> > + *
> > + * Description:
> > + *    Creates the network socket and maps it to a file descriptor
> > + **/
> 
> Seems sligtly inaccurate. socket listen create a listening socket...
> the description is quite obvious, so this kind of function doesn't
> really need a kernel-autodoc comment.

Consistency.

> 
> >  static int socket_listen(struct sockaddr *saddr, struct socket **res)
> >  {
> >  	struct socket *sock;
> >  	int ret, fd;
> > -	
> > +
> >  	ret = sock_create(saddr->sa_family, SOCK_STREAM, IPPROTO_TCP, &sock);
> >  	if (ret < 0)
> >  		return -1;
> 
> Nitpick: it's a documentation patch not, a whitespace patch.
> 
> > +/**
> > + * socket_listen_ip4
> > + *
> > + * Description:
> > + *    IPv4
> > + **/
> 
> pointless description...
> 
> >  static int socket_listen_ip4(int port, struct socket **res)
> >  {
> >  	struct sockaddr_in saddr4 = {
> > @@ -62,6 +74,12 @@
> >  	return socket_listen((struct sockaddr *) &saddr4, res);
> >  }
> >  
> > +/**
> > + * socket_listen_ip6
> > + *
> > + * Description:
> > + *    IPv6
> > + **/
> 
> ditto
> 
> > +/**
> > + * accept_connection
> > + *
> > + * Description:
> > + *    Once kcomd's sockets receive a new connection attempt,
> > + *    the connection is accepted, the remote IP address is
> > + *    retrieved, the file descriptor is mapped and the
> > + *    kcom node is created with this information.
> > + **/
> 
> do we really need a comment here as well ?

I think so. 

I don't understand the desire to minimize the comments. They aren't the
best comments, but they're accurate, and if anything, aren't detailed
enough.

> 
> > +/**
> > + * kcomd_thread
> > + *
> > + * Description:
> > + *    kcomd - kernel thread that handles the communications.
> > + *    Creates the memory slabs.
> > + *    Once the pkt has been sent, its memory is freed.
> > + *    Maps new connections to file descriptors.
> > + *    Waits for incoming data, signals from processes
> > + *    or any data that is ready to be sent.
> > + *    Also cleans up memory and any open sockets and
> > + *    file descriptors on exit.
> > + **/
> 
> Except that is make reference to some code that is not there yet (which
> means it should had come after the code change, anyway ..),
> I'm appling this one.

Yes, the comments describe what the function is supposed to do.  If the
comments don't match the code, that's a bug that needs to be fixed.

> 
> > +/**
> > + * mig_send_fp
> > + *
> > + * Description:
> > + *    Sends the process floating point information(?) to the other node.
> > + **/
> 
> I would rather change the name of function to mig_send_floating_point
> instead of documenting such a trivia.
> 
> I'm only commiting the kcomd_thread documentation... that's the only
> that seems really useful, all others hooks are NACKed,
> 
> 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