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

[Matt D. <Mat...@se...>]

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:=20
> 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
> >   **/
>=20
> why is that moving the short description into the long description field =
?
> specially since the text is the same.

Consistency.

>=20
> > [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
> >   **/
>=20
> (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.

>=20
> > [snip]
> > Index: linux/hpc/kcomd.c
> > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
> > --- 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>
> > =20
> > +/**
> > + * socket_listen
> > + *
> > + * Description:
> > + *    Creates the network socket and maps it to a file descriptor
> > + **/
>=20
> 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.

>=20
> >  static int socket_listen(struct sockaddr *saddr, struct socket **res)
> >  {
> >  	struct socket *sock;
> >  	int ret, fd;
> > -=09
> > +
> >  	ret =3D sock_create(saddr->sa_family, SOCK_STREAM, IPPROTO_TCP, =
&sock);
> >  	if (ret < 0)
> >  		return -1;
>=20
> Nitpick: it's a documentation patch not, a whitespace patch.
>=20
> > +/**
> > + * socket_listen_ip4
> > + *
> > + * Description:
> > + *    IPv4
> > + **/
>=20
> pointless description...
>=20
> >  static int socket_listen_ip4(int port, struct socket **res)
> >  {
> >  	struct sockaddr_in saddr4 =3D {
> > @@ -62,6 +74,12 @@
> >  	return socket_listen((struct sockaddr *) &saddr4, res);
> >  }
> > =20
> > +/**
> > + * socket_listen_ip6
> > + *
> > + * Description:
> > + *    IPv6
> > + **/
>=20
> ditto
>=20
> > +/**
> > + * 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.
> > + **/
>=20
> do we really need a comment here as well ?

I think so.=20

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.

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

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

Matt