libcldc,cldcli: Use humanized error messages

PATCH In review Pete Zaitcev 2 participants 4 messages Aug 27, 2009 Library code

lore

Share & tools

lore

Pete Zaitcev

author

2009-08-27 02:06 UTC #0 lore raw

PATCH 1/1 libcldc,cldcli: Use humanized error messages

Signed-off-by Pete Zaitcev <[email protected]>

include/cld_msg.h +1 -0

index 01bda16..e4c8f28 100644
@@ -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__ */

lib/common.c +26 -0

index fac652b..57f8696 100644
@@ -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)";
+	}
+}
+

tools/cldcli.c +5 ~~-24~~

index 635bc6e..eb4ebc4 100644
@@ -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, ...)

Jeff Garzik

2009-08-27 02:26 UTC #1 lore raw

Quoted 1 line · Pete Zaitcev

Signed-off-by Pete Zaitcev<[email protected]>

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

commit 469aeb041c69c43dda1051e5911c47b5dbc78721

Author: Pete Zaitcev <[email protected]>
Date:   Wed Aug 26 22:19:02 2009 -0400

libcldc, cldcli: Use humanized error messages

Signed-off-by Pete Zaitcev <[email protected]>

[slight change, error messages kept in array -jg]

Signed-off-by Jeff Garzik <[email protected]>

include/cld_msg.h +1 -0

index 01bda16..e4c8f28 100644
@@ -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__ */

lib/common.c +34 -0

index fac652b..a4eda54 100644
@@ -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];
+}
+

tools/cldcli.c +5 ~~-24~~

index 635bc6e..eb4ebc4 100644
@@ -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, ...)

Pete Zaitcev

author

2009-08-27 02:50 UTC #2 lore raw

Quoted 8 lines · Jeff Garzik

Quoted 1 line · Pete Zaitcev
Signed-off-by Pete Zaitcev<[email protected]>
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

Jeff Garzik

2009-08-27 03:04 UTC #3 lore raw

Quoted 21 lines · Pete Zaitcev

Quoted 8 lines · Jeff Garzik
Quoted 1 line · Pete Zaitcev
Signed-off-by Pete Zaitcev<[email protected]>
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:
lib/common.c +4 -0
+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:

lib/common.c void __cld_rand64(void *p) · line 54

Quoted 2 lines

lib/common.c +2 -0

+	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