[Patch] libcldc: transition to applog, phase 1

[Patch] libcldc: transition to applog, phase 1

[Pete Zaitcev]

This patch transitions from printf-like to syslog-like API for
libcldc diagnostics. In the future, this allows to drop app_log()
from chunkd (tabled didn't even grow it).

There's a small complication: one stdarg function cannot call
another, so we cannot just insert some shims... Or actually, we can.
The trick is to use printf into a local buffer, and then pass that down.
All of that is strictly temporary. If I'm not missing something, once
applications switch over, we'll just kill all of it and return to
what it was, only with syslog-like functions. I didn't even adjust
whitespace of ->app_log arguments as a pledge that it has to change
again anyway.

Long story short, this should be safe to build in Koji without
resorting to chain builds.

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

diff --git a/include/cldc.h b/include/cldc.h
index f625d5e..712e7c7 100644
--- a/include/cldc.h
+++ b/include/cldc.h
@@ -79,6 +79,7 @@ struct cldc_ops {
 	void		(*event)(void *private, struct cldc_session *,
 				 struct cldc_fh *, uint32_t);
 	void		(*printf)(const char *fmt, ...);
+	void		(*errlog)(int prio, const char *fmt, ...);
 };
 
 /** a single CLD client session */
@@ -88,7 +89,8 @@ struct cldc_session {
 	bool		verbose;
 
 	const struct cldc_ops *ops;
-	void		(*act_log)(const char *fmt, ...);
+	void		(*act_log)(struct cldc_session *,
+				   int prio, const char *fmt, ...);
 	void		*private;
 
 	uint8_t		addr[64];		/* server address */
diff --git a/lib/cldc.c b/lib/cldc.c
index dbb367c..072c995 100644
--- a/lib/cldc.c
+++ b/lib/cldc.c
@@ -35,6 +35,7 @@
 #include <glib.h>
 #include <cld-private.h>
 #include <cldc.h>
+#include <syslog.h>
 
 enum {
 	CLDC_MSG_EXPIRE		= 5 * 60,
@@ -80,12 +81,38 @@ static size_t strnlen(const char *s, size_t maxlen)
 #define EBADE 52
 #endif
 
-void cldc_log(const char *fmt, ...)
+static void cldc_stdlog(struct cldc_session *sess, int prio, const char *fmt, ...)
 {
+	char buf[200];
 	va_list ap;
 
 	va_start(ap, fmt);
-	vfprintf(stderr, fmt, ap);
+	vsnprintf(buf, 200, fmt, ap);
+	fprintf(stderr, "%s\n", buf);
+	va_end(ap);
+}
+
+static void cldc_oldlog(struct cldc_session *sess,
+			int prio, const char *fmt, ...)
+{
+	char buf[200];
+	va_list ap;
+
+	va_start(ap, fmt);
+	vsnprintf(buf, 200, fmt, ap);
+	sess->ops->printf("%s\n", buf);
+	va_end(ap);
+}
+
+static void cldc_applog(struct cldc_session *sess,
+			int prio, const char *fmt, ...)
+{
+	char buf[200];
+	va_list ap;
+
+	va_start(ap, fmt);
+	vsnprintf(buf, 200, fmt, ap);
+	sess->ops->errlog(prio, "%s", buf);
 	va_end(ap);
 }
 
@@ -109,7 +136,7 @@ static int ack_seqid(struct cldc_session *sess, uint64_t seqid_le)
 	memcpy(resp, &def_msg_ack, sizeof(*resp));
 
 	if (!authsign(sess, pkt, pkt_len)) {
-		sess->act_log("authsign failed 2\n");
+		sess->act_log(sess, LOG_INFO, "authsign failed 2");
 		return -1;
 	}
 
@@ -134,14 +161,12 @@ static int cldc_rx_generic(struct cldc_session *sess,
 	while (tmp) {
 		req = tmp->data;
 
-#if 1 /* too verbose */
 		if (sess->verbose)
-			sess->act_log("rx_gen: comparing req->xid (%llu) with resp->xid_in (%llu)\n",
+			sess->act_log(sess, LOG_DEBUG, "rx_gen: comparing req->xid (%llu) with resp->xid_in (%llu)",
 			        (unsigned long long)
 					le64_to_cpu(req->xid),
 			        (unsigned long long)
 					le64_to_cpu(resp->xid_in));
-#endif
 
 		if (req->xid == resp->xid_in)
 			break;
@@ -152,10 +177,10 @@ static int cldc_rx_generic(struct cldc_session *sess,
 
 	if (req->done) {
 		if (sess->verbose)
-			sess->act_log("rx_gen: re-acking\n");
+			sess->act_log(sess, LOG_DEBUG, "rx_gen: re-acking");
 	} else {
 		if (sess->verbose)
-			sess->act_log("rx_gen: issuing completion, acking\n");
+			sess->act_log(sess, LOG_DEBUG, "rx_gen: issuing completion, acking");
 
 		req->done = true;
 
@@ -182,7 +207,7 @@ static int cldc_rx_ack_frag(struct cldc_session *sess,
 		return -1008;
 
 	if (sess->verbose)
-		sess->act_log("ack-frag: seqid %llu, want to ack",
+		sess->act_log(sess, LOG_DEBUG, "ack-frag: seqid %llu, want to ack",
 			      ack_msg->seqid);
 
 	tmp = sess->out_msg;
@@ -202,7 +227,8 @@ static int cldc_rx_ack_frag(struct cldc_session *sess,
 				continue;
 
 			if (sess->verbose)
-				sess->act_log("ack-frag: seqid %llu, expiring",
+				sess->act_log(sess, LOG_DEBUG,
+					      "ack-frag: seqid %llu, expiring",
 					      ack_msg->seqid);
 
 			req->pkt_info[i] = NULL;
@@ -247,7 +273,7 @@ static int cldc_rx_not_master(struct cldc_session *sess,
 			      const void *msgbuf,
 			      size_t buflen)
 {
-	sess->act_log("FIXME: not-master message received\n");
+	sess->act_log(sess, LOG_INFO, "FIXME: not-master message received");
 	return -1055;	/* FIXME */
 }
 
@@ -314,7 +340,8 @@ static bool authcheck(struct cldc_session *sess, const struct cld_packet *pkt,
 	     md, &md_len);
 
 	if (md_len != SHA_DIGEST_LENGTH)
-		sess->act_log("authsign BUG: md_len != SHA_DIGEST_LENGTH\n");
+		sess->act_log(sess, LOG_INFO,
+			      "authsign BUG: md_len != SHA_DIGEST_LENGTH");
 
 	if (memcmp(buf + buflen - SHA_DIGEST_LENGTH, md, SHA_DIGEST_LENGTH))
 		return false;
@@ -338,7 +365,8 @@ static bool authsign(struct cldc_session *sess, struct cld_packet *pkt,
 	     md, &md_len);
 
 	if (md_len != SHA_DIGEST_LENGTH)
-		sess->act_log("authsign BUG: md_len != SHA_DIGEST_LENGTH\n");
+		sess->act_log(sess, LOG_INFO,
+			      "authsign BUG: md_len != SHA_DIGEST_LENGTH");
 
 	memcpy(buf + (buflen - SHA_DIGEST_LENGTH), md, SHA_DIGEST_LENGTH);
 
@@ -423,7 +451,7 @@ int cldc_receive_pkt(struct cldc_session *sess,
 
 	if (pkt_len < (sizeof(*pkt) + SHA_DIGEST_LENGTH)) {
 		if (sess->verbose)
-			sess->act_log("receive_pkt: msg too short\n");
+			sess->act_log(sess, LOG_DEBUG, "receive_pkt: msg too short");
 		return -EPROTO;
 	}
 
@@ -433,24 +461,26 @@ int cldc_receive_pkt(struct cldc_session *sess,
 		if (msg->op == cmo_get) {
 			struct cld_msg_get_resp *dp;
 			dp = (struct cld_msg_get_resp *) msg;
-			sess->act_log("receive pkt: len %u, op cmo_get"
-				", seqid %llu, user %s, size %u\n",
+			sess->act_log(sess, LOG_DEBUG, "receive pkt: len %u, op %s"
+				", seqid %llu, user %s, size %u",
 				(unsigned int) pkt_len,
+				opstr(msg->op),
 				(unsigned long long) le64_to_cpu(pkt->seqid),
 				pkt->user,
 				le32_to_cpu(dp->size));
 		} else if (msg->op == cmo_new_sess) {
 			struct cld_msg_resp *dp;
 			dp = (struct cld_msg_resp *) msg;
-			sess->act_log("receive pkt: len %u, op cmo_new_sess"
-				", seqid %llu, user %s, xid_in %llu\n",
+			sess->act_log(sess, LOG_DEBUG, "receive pkt: len %u, op %s"
+				", seqid %llu, user %s, xid_in %llu",
 				(unsigned int) pkt_len,
+				opstr(msg->op),
 				(unsigned long long) le64_to_cpu(pkt->seqid),
 				pkt->user,
 				(unsigned long long) le64_to_cpu(dp->xid_in));
 		} else {
-			sess->act_log("receive pkt: len %u, "
-				"op %s, seqid %llu, user %s\n",
+			sess->act_log(sess, LOG_DEBUG, "receive pkt: len %u, op %s"
+				", seqid %llu, user %s",
 				(unsigned int) pkt_len,
 				opstr(msg->op),
 				(unsigned long long) le64_to_cpu(pkt->seqid),
@@ -460,14 +490,14 @@ int cldc_receive_pkt(struct cldc_session *sess,
 
 	if (memcmp(pkt->magic, CLD_PKT_MAGIC, sizeof(pkt->magic))) {
 		if (sess->verbose)
-			sess->act_log("receive_pkt: bad pkt magic\n");
+			sess->act_log(sess, LOG_DEBUG, "receive_pkt: bad pkt magic");
 		return -EPROTO;
 	}
 
 	/* check HMAC signature */
 	if (!authcheck(sess, pkt, pkt_len)) {
 		if (sess->verbose)
-			sess->act_log("receive_pkt: invalid auth\n");
+			sess->act_log(sess, LOG_DEBUG, "receive_pkt: invalid auth");
 		return -EACCES;
 	}
 
@@ -475,7 +505,8 @@ int cldc_receive_pkt(struct cldc_session *sess,
 	if (((sess->addr_len != net_addrlen) ||
 	    memcmp(sess->addr, net_addr, net_addrlen))) {
 		if (sess->verbose)
-			sess->act_log("receive_pkt: server address mismatch\n");
+			sess->act_log(sess, LOG_DEBUG,
+				      "receive_pkt: server address mismatch");
 		return -EBADE;
 	}
 
@@ -495,7 +526,7 @@ int cldc_receive_pkt(struct cldc_session *sess,
 
 	if ((sess->msg_buf_len + msglen) > CLD_MAX_MSG_SZ) {
 		if (sess->verbose)
-			sess->act_log("receive_pkt: bad pkt length\n");
+			sess->act_log(sess, LOG_DEBUG, "receive_pkt: bad pkt length");
 		return -EPROTO;
 	}
 
@@ -504,7 +535,7 @@ int cldc_receive_pkt(struct cldc_session *sess,
 
 	if (memcmp(msg->magic, CLD_MSG_MAGIC, sizeof(msg->magic))) {
 		if (sess->verbose)
-			sess->act_log("receive_pkt: bad msg magic\n");
+			sess->act_log(sess, LOG_DEBUG, "receive_pkt: bad msg magic");
 		return -EPROTO;
 	}
 
@@ -516,8 +547,8 @@ int cldc_receive_pkt(struct cldc_session *sess,
 			sess->next_seqid_in - CLDC_MSG_REMEMBER;
 
 		if (sess->verbose)
-			sess->act_log("receive_pkt: "
-					 "setting next_seqid_in to %llu\n",
+			sess->act_log(sess, LOG_DEBUG, "receive_pkt: "
+					 "setting next_seqid_in to %llu",
 					 (unsigned long long) seqid);
 	} else if (!no_seqid) {
 		if (seqid != sess->next_seqid_in) {
@@ -527,8 +558,9 @@ int cldc_receive_pkt(struct cldc_session *sess,
 				return ack_seqid(sess, pkt->seqid);
 
 			if (sess->verbose)
-				sess->act_log("receive_pkt: bad seqid %llu\n",
-					(unsigned long long) seqid);
+				sess->act_log(sess, LOG_DEBUG,
+					      "receive_pkt: bad seqid %llu",
+					      (unsigned long long) seqid);
 			return -EBADSLT;
 		}
 		sess->next_seqid_in++;
@@ -841,7 +873,13 @@ int cldc_new_sess(const struct cldc_ops *ops,
 
 	sess->private = private;
 	sess->ops = ops;
-	sess->act_log = ops->printf ? ops->printf : cldc_log;
+	if (ops->printf) {	/* obsolete API in use XXX */
+		sess->act_log = cldc_oldlog;
+	} else if (ops->errlog) {
+		sess->act_log = cldc_applog;
+	} else {
+		sess->act_log = cldc_stdlog;
+	}
 	sess->fh = g_array_sized_new(FALSE, TRUE, sizeof(struct cldc_fh), 16);
 	strcpy(sess->user, user);
 	strcpy(sess->secret_key, secret_key);

Re: [Patch] libcldc: transition to applog, phase 1

[Jeff Garzik]

Pete Zaitcev wrote:
> This patch transitions from printf-like to syslog-like API for
> libcldc diagnostics. In the future, this allows to drop app_log()
> from chunkd (tabled didn't even grow it).
> 
> There's a small complication: one stdarg function cannot call
> another, so we cannot just insert some shims... Or actually, we can.
> The trick is to use printf into a local buffer, and then pass that down.
> All of that is strictly temporary. If I'm not missing something, once
> applications switch over, we'll just kill all of it and return to
> what it was, only with syslog-like functions. I didn't even adjust
> whitespace of ->app_log arguments as a pledge that it has to change
> again anyway.
> 
> Long story short, this should be safe to build in Koji without
> resorting to chain builds.
> 
> Signed-off-by: Pete Zaitcev <zaitcev@redhat.com>

applied