From: Matt D. <Mat...@se...> - 2006-10-08 22:14:33
|
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 |