[Patch] libcldc,cldcli: Use humanized error messages

[Patch] libcldc,cldcli: Use humanized error messages

[Pete Zaitcev]

Signed-off-by: Pete Zaitcev <zaitcev@redhat.com>

diff --git a/include/cld_msg.h b/include/cld_msg.h
index 01bda16..e4c8f28 100644
--- a/include/cld_msg.h
+++ b/include/cld_msg.h
@@ -257,5 +257,6 @@ struct cld_msg_event {
 
 extern unsigned long long cld_sid2llu(const uint8_t *sid);
 extern void __cld_rand64(void *p);
+extern const char *cld_errstr(enum cle_err_codes ecode);
 
 #endif /* __CLD_MSG_H__ */
diff --git a/lib/common.c b/lib/common.c
index fac652b..57f8696 100644
--- a/lib/common.c
+++ b/lib/common.c
@@ -19,3 +19,29 @@ void __cld_rand64(void *p)
 	v[1] = rand();
 }
 
+const char *cld_errstr(enum cle_err_codes ecode)
+{
+	switch (ecode) {
+	case CLE_OK:		return "Success";
+	case CLE_SESS_EXISTS:	return "Session exists";
+	case CLE_SESS_INVAL:	return "Invalid session";
+	case CLE_DB_ERR:	return "Database error";
+	case CLE_BAD_PKT:	return "Invalid/corrupted packet";
+	case CLE_INODE_INVAL:	return "Invalid inode number";
+	case CLE_NAME_INVAL:	return "Invalid file name";
+	case CLE_OOM:		return "Server out of memory";
+	case CLE_FH_INVAL:	return "Invalid file handle";
+	case CLE_DATA_INVAL:	return "Invalid data packet";
+	case CLE_LOCK_INVAL:	return "Invalid lock";
+	case CLE_LOCK_CONFLICT:	return "Conflicting lock held";
+	case CLE_LOCK_PENDING:	return "Lock waiting to be acquired";
+	case CLE_MODE_INVAL:	return "Operation incompatible with file mode";
+	case CLE_INODE_EXISTS:	return "File exists";
+	case CLE_DIR_NOTEMPTY:	return "Directory not empty";
+	case CLE_INTERNAL_ERR:	return "Internal error";
+	case CLE_TIMEOUT:	return "Session timed out";
+	case CLE_SIG_INVAL:	return "Bad HMAC signature";
+	default:		return "(unknown)";
+	}
+}
+
diff --git a/tools/cldcli.c b/tools/cldcli.c
index 635bc6e..eb4ebc4 100644
--- a/tools/cldcli.c
+++ b/tools/cldcli.c
@@ -66,9 +66,11 @@ struct creq {
 	struct cp_fc_info	*cfi;
 };
 
+enum { CRESP_MSGSZ = 64 };
+
 struct cresp {
 	enum thread_codes	tcode;
-	char			msg[64];
+	char			msg[CRESP_MSGSZ];
 	union {
 		size_t		file_len;
 		unsigned int	n_records;
@@ -117,31 +119,10 @@ static error_t parse_opt (int key, char *arg, struct argp_state *state);
 
 static const struct argp argp = { options, parse_opt, NULL, doc };
 
-static const char *names_cle_err[] = {
-	[CLE_OK]		= "CLE_OK",
-	[CLE_SESS_EXISTS]	= "CLE_SESS_EXISTS",
-	[CLE_SESS_INVAL]	= "CLE_SESS_INVAL",
-	[CLE_DB_ERR]		= "CLE_DB_ERR",
-	[CLE_BAD_PKT]		= "CLE_BAD_PKT",
-	[CLE_INODE_INVAL]	= "CLE_INODE_INVAL",
-	[CLE_NAME_INVAL]	= "CLE_NAME_INVAL",
-	[CLE_OOM]		= "CLE_OOM",
-	[CLE_FH_INVAL]		= "CLE_FH_INVAL",
-	[CLE_DATA_INVAL]	= "CLE_DATA_INVAL",
-	[CLE_LOCK_INVAL]	= "CLE_LOCK_INVAL",
-	[CLE_LOCK_CONFLICT]	= "CLE_LOCK_CONFLICT",
-	[CLE_LOCK_PENDING]	= "CLE_LOCK_PENDING",
-	[CLE_MODE_INVAL]	= "CLE_MODE_INVAL",
-	[CLE_INODE_EXISTS]	= "CLE_INODE_EXISTS",
-	[CLE_DIR_NOTEMPTY]	= "CLE_DIR_NOTEMPTY",
-	[CLE_INTERNAL_ERR]	= "CLE_INTERNAL_ERR",
-	[CLE_TIMEOUT]		= "CLE_TIMEOUT",
-	[CLE_SIG_INVAL]		= "CLE_SIG_INVAL",
-};
-
 static void errc_msg(struct cresp *cresp, enum cle_err_codes errc)
 {
-	strcpy(cresp->msg, names_cle_err[errc]);
+	strncpy(cresp->msg, cld_errstr(errc), CRESP_MSGSZ);
+	cresp->msg[CRESP_MSGSZ-1] = 0;
 }
 
 static void applog(int prio, const char *fmt, ...)

Re: [Patch] libcldc,cldcli: Use humanized error messages

[Jeff Garzik]

[-- Attachment #1: Type: text/plain, Size: 345 bytes --]

On 08/26/2009 10:06 PM, Pete Zaitcev wrote:
> Signed-off-by: Pete Zaitcev<zaitcev@redhat.com>

applied a modified version, see attached...  after that short IRC 
discussion, I think an array with a range check would be nicer than 
coding it (though the C compiler should make a nice, efficient table 
from 'switch' statement for us...)

	Jeff



[-- Attachment #2: patch --]
[-- Type: text/plain, Size: 3742 bytes --]

commit 469aeb041c69c43dda1051e5911c47b5dbc78721
Author: Pete Zaitcev <zaitcev@redhat.com>
Date:   Wed Aug 26 22:19:02 2009 -0400

    libcldc, cldcli: Use humanized error messages
    
    Signed-off-by: Pete Zaitcev <zaitcev@redhat.com>
    
    [slight change, error messages kept in array -jg]
    Signed-off-by: Jeff Garzik <jgarzik@redhat.com>

diff --git a/include/cld_msg.h b/include/cld_msg.h
index 01bda16..e4c8f28 100644
--- a/include/cld_msg.h
+++ b/include/cld_msg.h
@@ -257,5 +257,6 @@ struct cld_msg_event {
 
 extern unsigned long long cld_sid2llu(const uint8_t *sid);
 extern void __cld_rand64(void *p);
+extern const char *cld_errstr(enum cle_err_codes ecode);
 
 #endif /* __CLD_MSG_H__ */
diff --git a/lib/common.c b/lib/common.c
index fac652b..a4eda54 100644
--- a/lib/common.c
+++ b/lib/common.c
@@ -4,6 +4,9 @@
 #include <cld-private.h>
 #include "cld_msg.h"
 
+/* duplicated from tools/cldcli.c; put in common header somewhere? */
+#define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]))
+
 unsigned long long cld_sid2llu(const uint8_t *sid)
 {
 	const uint64_t *v_le = (const uint64_t *) sid;
@@ -19,3 +22,34 @@ void __cld_rand64(void *p)
 	v[1] = rand();
 }
 
+static const char *cld_errlist[] =
+{
+	[CLE_OK]		= "Success",
+	[CLE_SESS_EXISTS]	= "Session exists",
+	[CLE_SESS_INVAL]	= "Invalid session",
+	[CLE_DB_ERR]		= "Database error",
+	[CLE_BAD_PKT]		= "Invalid/corrupted packet",
+	[CLE_INODE_INVAL]	= "Invalid inode number",
+	[CLE_NAME_INVAL]	= "Invalid file name",
+	[CLE_OOM]		= "Server out of memory",
+	[CLE_FH_INVAL]		= "Invalid file handle",
+	[CLE_DATA_INVAL]	= "Invalid data packet",
+	[CLE_LOCK_INVAL]	= "Invalid lock",
+	[CLE_LOCK_CONFLICT]	= "Conflicting lock held",
+	[CLE_LOCK_PENDING]	= "Lock waiting to be acquired",
+	[CLE_MODE_INVAL]	= "Operation incompatible with file mode",
+	[CLE_INODE_EXISTS]	= "File exists",
+	[CLE_DIR_NOTEMPTY]	= "Directory not empty",
+	[CLE_INTERNAL_ERR]	= "Internal error",
+	[CLE_TIMEOUT]		= "Session timed out",
+	[CLE_SIG_INVAL]		= "Bad HMAC signature",
+};
+
+const char *cld_errstr(enum cle_err_codes ecode)
+{
+	if (ecode >= ARRAY_SIZE(cld_errlist))
+		return "(unknown)";
+
+	return cld_errlist[ecode];
+}
+
diff --git a/tools/cldcli.c b/tools/cldcli.c
index 635bc6e..eb4ebc4 100644
--- a/tools/cldcli.c
+++ b/tools/cldcli.c
@@ -66,9 +66,11 @@ struct creq {
 	struct cp_fc_info	*cfi;
 };
 
+enum { CRESP_MSGSZ = 64 };
+
 struct cresp {
 	enum thread_codes	tcode;
-	char			msg[64];
+	char			msg[CRESP_MSGSZ];
 	union {
 		size_t		file_len;
 		unsigned int	n_records;
@@ -117,31 +119,10 @@ static error_t parse_opt (int key, char *arg, struct argp_state *state);
 
 static const struct argp argp = { options, parse_opt, NULL, doc };
 
-static const char *names_cle_err[] = {
-	[CLE_OK]		= "CLE_OK",
-	[CLE_SESS_EXISTS]	= "CLE_SESS_EXISTS",
-	[CLE_SESS_INVAL]	= "CLE_SESS_INVAL",
-	[CLE_DB_ERR]		= "CLE_DB_ERR",
-	[CLE_BAD_PKT]		= "CLE_BAD_PKT",
-	[CLE_INODE_INVAL]	= "CLE_INODE_INVAL",
-	[CLE_NAME_INVAL]	= "CLE_NAME_INVAL",
-	[CLE_OOM]		= "CLE_OOM",
-	[CLE_FH_INVAL]		= "CLE_FH_INVAL",
-	[CLE_DATA_INVAL]	= "CLE_DATA_INVAL",
-	[CLE_LOCK_INVAL]	= "CLE_LOCK_INVAL",
-	[CLE_LOCK_CONFLICT]	= "CLE_LOCK_CONFLICT",
-	[CLE_LOCK_PENDING]	= "CLE_LOCK_PENDING",
-	[CLE_MODE_INVAL]	= "CLE_MODE_INVAL",
-	[CLE_INODE_EXISTS]	= "CLE_INODE_EXISTS",
-	[CLE_DIR_NOTEMPTY]	= "CLE_DIR_NOTEMPTY",
-	[CLE_INTERNAL_ERR]	= "CLE_INTERNAL_ERR",
-	[CLE_TIMEOUT]		= "CLE_TIMEOUT",
-	[CLE_SIG_INVAL]		= "CLE_SIG_INVAL",
-};
-
 static void errc_msg(struct cresp *cresp, enum cle_err_codes errc)
 {
-	strcpy(cresp->msg, names_cle_err[errc]);
+	strncpy(cresp->msg, cld_errstr(errc), CRESP_MSGSZ);
+	cresp->msg[CRESP_MSGSZ-1] = 0;
 }
 
 static void applog(int prio, const char *fmt, ...)

Re: [Patch] libcldc,cldcli: Use humanized error messages

[Pete Zaitcev]

On Wed, 26 Aug 2009 22:26:50 -0400, Jeff Garzik <jeff@garzik.org> wrote:
> On 08/26/2009 10:06 PM, Pete Zaitcev wrote:

> > Signed-off-by: Pete Zaitcev<zaitcev@redhat.com>
> 
> applied a modified version, see attached...  after that short IRC 
> discussion, I think an array with a range check would be nicer than 
> coding it (though the C compiler should make a nice, efficient table 
> from 'switch' statement for us...)

I don't think it's enough. Look:

+static const char *cld_errlist[] =
+{
+	[CLE_OK]		= "Success",
+	[CLE_SESS_EXISTS]	= "Session exists",

When you do indexed initializations, it's possible to create
empty array elements (currently there are none because indexes
are contiguous). So, the following needs a NULL check:

+	return cld_errlist[ecode];
+}

-- Pete

Re: [Patch] libcldc,cldcli: Use humanized error messages

[Jeff Garzik]

On 08/26/2009 10:50 PM, Pete Zaitcev wrote:
> On Wed, 26 Aug 2009 22:26:50 -0400, Jeff Garzik<jeff@garzik.org>  wrote:
>> On 08/26/2009 10:06 PM, Pete Zaitcev wrote:
>
>>> Signed-off-by: Pete Zaitcev<zaitcev@redhat.com>
>>
>> applied a modified version, see attached...  after that short IRC
>> discussion, I think an array with a range check would be nicer than
>> coding it (though the C compiler should make a nice, efficient table
>> from 'switch' statement for us...)
>
> I don't think it's enough. Look:
>
> +static const char *cld_errlist[] =
> +{
> +	[CLE_OK]		= "Success",
> +	[CLE_SESS_EXISTS]	= "Session exists",
>
> When you do indexed initializations, it's possible to create
> empty array elements (currently there are none because indexes
> are contiguous). So, the following needs a NULL check:
>
> +	return cld_errlist[ecode];
> +}

True in general, but do you actually see a hole?

It seems to me that our error list will always be sequential, without 
holes.  Even if we obsolete an error, we still need to keep it around in 
the headers and cld_errlist[].  And new error codes will go on the end 
of the list...

	Jeff