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

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.

> [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 ???

> [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.

>  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 ?

> +/**
> + * 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.

> +/**
> + * 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,
-- 
Vincent Hanquez

-------------------------------------------------------------------------
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