On Thu, Apr 11, 2013 at 08:03:15PM -0400, Zvi Gilboa wrote:
Thanks for the explanation! Since crypt.c is likely to be visited
by anyone going through the crypt-related code, I'd suggest placing
the comment there (proposed comment attached).
[...]
>From b9d9d4c46486b4f0e77cf7b4455de8ba87c9f5ef Mon Sep 17 00:00:00 2001
From: Zvi Gilboa <zgilboa@zgilboa-S14Y-linux.(none)>
Date: Thu, 11 Apr 2013 19:52:05 -0400
Subject: [PATCH] Committer: Zvi Gilboa <zgilboa@zgilboa-S14Y-linux.(none)>
On branch crypt_data_comment
---
src/crypt/crypt.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/src/crypt/crypt.c b/src/crypt/crypt.c
index f1e310f..fdc4427 100644
--- a/src/crypt/crypt.c
+++ b/src/crypt/crypt.c
@@ -1,6 +1,12 @@
#include <unistd.h>
#include <crypt.h>
+/***
+ struct crypt_data --> the entire structure is used as a char[] buffer
+ int initialized --> member exists to satisfy API requirements, currently not used
+ char __buf[256]; --> large enough for any future password hash
+ ***/
+
I understand your motivation, but I don't want to introduce comments
like this that duplicate definitions from elsewhere. I think the
comments should be in crypt.c and crypt_r.c, and should be something
along the line of:
(crypt.c) This buffer is sufficiently large for all
currently-supported hash types. It needs to be updated if longer
hashes are added. The cast to struct crypt_data * is purely to meet
the public API requirements of the crypt_r function; the
implementation of crypt_r uses the object purely as a char buffer.
(crypt_r.c) Per the crypt_r API, the caller has provided a pointer to
struct crypt_data; however, this implementation does not use the
structure to store any internal state, and treats it purely as a char
buffer for storing the result.
Would these comments have been sufficient for you to understand what
was going on when you first read the source? If so, I'll go ahead and
add them; if not, what would you like to see improved?