Re: the evils of the CLD api

[Pete Zaitcev <zaitcev@redhat.com>]

On Fri, 15 Jan 2010 18:40:44 -0800
Colin McCabe <cmccabe@alumni.cmu.edu> wrote:

> 1) when starting the cld session, the user code could register a
> "vtable" containing a bunch of different callbacks.
> That way all GET responses for the session could flow through
> my_get_response(), and so on.

I'm not sure if that would be much better than what we have now,
so I don't see a big pay-off from switching to it.

> I think a non-blocking API, in some form, should probably be
> available, though, in case people need it. We may not be able to
> anticipate all the uses that CLD is put to.

The easiest way to do this is to leave cldc intact.

> 2) provide a blocking API that users could take advantage of.
> I think for a lot of simple programs, programmers would be happy to
> spawn a thread per CLD session. In fact, I'm not aware of any programs
> that want to spawn a lot of cld sessions (maybe someone can point out
> one.)

Here's what I have for tonight:

commit e011b6c073d1dafd506dc36c5bb2d7ef122f65d7
Author: Master <zaitcev@lembas.zaitcev.lan>
Date:   Fri Jan 15 23:26:08 2010 -0700

    Checkpoint WIP for ncld.h.

diff --git a/include/ncld.h b/include/ncld.h
new file mode 100644
index 0000000..ed52f1d
--- /dev/null
+++ b/include/ncld.h
@@ -0,0 +1,58 @@
+#ifndef __NCLD_H__
+#define __NCLD_H__
+
+/*
+ * Copyright 2009 Red Hat, Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; see the file COPYING.  If not, write to
+ * the Free Software Foundation, 675 Mass Ave, Cambridge, MA 02139, USA.
+ *
+ */
+/*
+ * The ncld.h API is a replacement for cldc.h. Do not include both.
+ *
+ * We do not believe into making "internal" structures "opaque"
+ * with pointers to void. Therefore, this header might want include
+ * some legacy definitions or whatnot, but users do not need to.
+ */
+#include <glib.h>
+#include <cldc.h>
+
+struct ncld_fh {
+};
+
+struct ncld_sess {
+	char *host;
+	unsigned short port;
+	// GMutex *mutex; mutex = g_mutex_new(); g_mutex_lock(mutex);
+	GThread *thread;
+	struct cldc_udp *udp;
+	void (*event)(struct ncld_sess *, void *, unsigned int);
+	void *event_arg;
+};
+
+extern struct ncld_sess *ncld_sess_open(const char *host, const char *port,
+	int *error,
+	void (*event)(struct ncld_sess *, void *, unsigned int), void *ev_arg);
+extern struct ncld_fh *ncld_open(struct ncld_sess *s, const char *fname,
+	unsigned int mode, int *error, unsigned int events,
+	void (*event)(struct ncld_fh *, void *, unsigned int), void *ev_arg);
+extern long ncld_read(struct ncld_fh *, void *data, long len);
+extern long ncld_write(struct ncld_fh *, const void *data, long len);
+extern int ncld_lock(struct ncld_fh *);
+extern void ncld_unlock(struct ncld_fh *);
+extern void ncld_close(struct ncld_fh *);
+extern void ncld_sess_close(struct ncld_sess *s);
+extern void ncld_init(void);
+
+#endif /* __NCLD_H__ */
diff --git a/lib/cldc.c b/lib/cldc.c
index bc4b48c..514b0f7 100644
--- a/lib/cldc.c
+++ b/lib/cldc.c
@@ -1378,6 +1378,366 @@ char *cldc_dirent_name(struct cld_dirent_cur *dc)
 	return s;
 }
 
+#if 0 /* temporarily, while checking cld_timer_foo in libtimer */
+/*
+ * On error, return the code (not negated code like a kernel function would).
+ */
+static int ncld_getsrv(char *hostp, unsigned short *portp,
+		       struct hail_log *hlog)
+{
+	enum { hostsz = 64 };
+	char hostb[hostsz];
+	GList *host_list = NULL;
+	GList *tmp;
+	struct cldc_host *hp;
+
+	if (gethostname(hostb, hostsz-1) < 0)
+		return errno;
+	hostb[hostsz-1] = 0;
+
+	if (cldc_getaddr(&host_list, hostb, hlog))
+		return 1001;
+
+	/*
+	 * This is a good place to compare weights and priorities, maybe later.
+	 * For now, just grab first host in the list.
+	 */
+	hp = host_list->data;		/* cannot be NULL if success above */
+	*hostp = hp->host;
+	*portp = hp->port;
+
+	for (tmp = host_list; tmp; tmp = tmp->next) {
+		hp = tmp->data;
+		free(hp->host);
+		free(hp);
+	}
+	g_list_free(host_list);
+	return 0;
+}
+
+static int ncld_gethost(char *hostp, unsigned short *portp,
+			const char *hostname, const char *portstr)
+{
+	long n;
+
+	n = strtol(portstr, NULL, 10);
+	if (n <= 0 || n >= 65536)
+		return EINVAL;
+	*portp = n;
+
+	if (!(*hostp = strdup(hostname)))
+		return ENOMEM;
+
+	return 0;
+}
+
+static void ncld_udp_timer_event(int fd, short events, void *userdata)
+ <------- timer
+{
+	struct cld_session *sp = userdata;
+	struct cldc_udp *udp = sp->lib;
+
+	if (udp->cb)
+		udp->cb(udp->sess, udp->cb_private);
+}
+
+static gpointer ncld_sess_thr(gpointer data)
+{
+	struct ncld_sess *nsp = data;
+	int ufd;
+	fd_set rset;
+	struct timeval tm;
+	time_t tmo;
+	int rc;
+
+	for (;;) {
+		if (!nsp->udp)
+			......
+
+		ufd = nsp->udp->fd;
+		FD_ZERO(&rset);
+		FD_SET(ufd, &rset);
+
+		tmo = cld_timers_run(&nsp->tlist);
+		if (tmo) {
+			tm.tv_sec = tmo;
+			tm.tv_usec = 0;
+			rc = select(ufd + 1, &rset, NULL, NULL, &tm);
+			if (rc < 0) {
+				fprintf(stderr, "select: error\n");
+				exit(1);
+			}
+			if (rc == 0)
+				continue;
+		} else {
+			rc = select(ufd + 1, &rset, NULL, NULL, NULL);
+			if (rc <= 0) {
+				fprintf(stderr, "select: nfd %d\n", rc);
+				exit(1);
+			}
+		}
+
+		if (FD_ISSET(ufd, &rset)) {
+			rc = cldc_udp_receive_pkt(udp);
+			if (rc) {
+				fprintf(stderr,
+					"cldc_udp_receive_pkt: error %d\n", rc);
+				exit(1);
+			}
+		} else {
+			fprintf(stderr, "noevent\n");
+			exit(1);
+		}
+	}
+
+	return NULL;
+}
+
+/*
+ * Safe the thread so that we can tear down the session.
+ */
+static void ncld_thr_poison(struct ncld_sess *nsp)
+{
+}
+
+/*
+ * Ask the thread to exit and wait until it does.
+ */
+static void ncld_thr_end(struct ncld_sess *nsp)
+{
+	.......
+}
+
+static int ncld_new_sess(struct cldc_call_opts *copts, enum cle_err_codes errc)
+{
+	......
+}
+
+static bool ncld_p_timer_ctl(void *priv, bool add,
+			     int (*cb)(struct cldc_session *, void *),
+			     void *cb_priv, time_t secs)
+{
+	struct ncld_sess *nsp = priv;
+	struct cldc_udp *udp = nsp->udp;
+	struct timeval tv = { secs, 0 };
+
+	if (add) {
+		udp->cb = cb;
+		udp->cb_private = cb_priv;
+		return cld_timer_add(&nsp->lib_timer, &tv) == 0;
+	} else {
+		return cld_timer_del(&nsp->lib_timer) == 0;
+	}
+}
+
+static int ncld_p_pkt_send(void *priv, const void *addr, size_t addrlen,
+			       const void *buf, size_t buflen)
+{
+	struct ncld_sess *nsp = priv;
+	return cldc_udp_pkt_send(nsp->udp, addr, addrlen, buf, buflen);
+}
+
+static void ncld_p_event(void *priv, struct cldc_session *csp,
+			 struct cldc_fh *fh, uint32_t what)
+{
+	struct ncld_sess *nsp = priv;
+	int newactive;
+
+	if (what == CE_SESS_FAILED) {
+		sp->sess_open = false;
+		if (sp->lib->sess != csp)
+			applog(LOG_ERR, "Stray session failed, sid " SIDFMT,
+			       SIDARG(csp->sid));
+		else
+			applog(LOG_ERR, "Session failed, sid " SIDFMT,
+			       SIDARG(csp->sid));
+		// cld_timer_del(&sp->tm);
+		sp->lib->sess = NULL;
+		newactive = cldu_nextactive(sp);
+		if (cldu_set_cldc(sp, newactive))
+			return;
+		// cld_timer_add(&sp->tm, &cldc_to_delay);
+	} else {
+		if (csp)
+			applog(LOG_INFO, "cldc event 0x%x sid " SIDFMT,
+			       what, SIDARG(csp->sid));
+		else
+			applog(LOG_INFO, "cldc event 0x%x no sid", what);
+	}
+}
+
+static struct cldc_ops ncld_ops = {
+	.timer_ctl	= ncld_p_timer_ctl,
+	.pkt_send	= ncld_p_pkt_send,
+	.event		= ncld_p_event,
+	.errlog		= NULL;
+};
+
+/*
+ * Initiating session may take a while, since we retry failures.
+ * We promise that eventually we return from this function.
+ *
+ * On error, returns NULL and sets the error code (system errno if below 1000,
+ * otherwise our own mysterious codes that you should just print in decimal).
+ *
+ * Keep in mind that the (*event)() is likely to be called on a context
+ * of the extra thread that we spawn. At least we won't steal some random
+ * context and monkey with signals. Also, (*event)() may be called before
+ * this function returns, with the session going down, for example.
+ * This is kind of dirty, but oh well. Maybe we'll fix this later.
+ *
+ * The lifetime of the hlog must be longer than that of the session.
+ *
+ * @param host Host name (NULL if resolving SRV records)
+ * @param port Port string (NULL if resolving SRV records)
+ * @param error Buffer for the error code
+ * @param event Session event function
+ * @param ev_arg User-supplied argument to the session event function
+ * @param cld_user The user identifier to be used to authentication
+ * @param cld_key The user key to be used to authentication
+ * @param hlog The log to which we (sadly) write, because we use cldc.
+ */
+struct ncld_sess *ncld_sess_open(const char *host, const char *port, int *error,
+	void (*event)(struct ncld_sess *, void *, unsigned int), void *ev_arg,
+	const char *cld_user, const char *cld_key,
+	struct hail_log *hlog)
+{
+	struct ncld_sess *nsp;
+	static GThread *thread;
+	struct cldc_call_opts copts;
+	GError *gerr;
+	int err;
+
+	err = ENOMEM;
+	nsp = malloc(sizeof(struct ncld_sess));
+	if (!nsp)
+		goto out_sesalloc;
+	memset(nsp, 0, sizeof(ncld_sess));
+
+	if (!host) {
+		err = EINVAL;
+		if (port)
+			goto out_portarg;
+		err = ncld_getsrv(&nsp->host, &nsp->port, hlog);
+		if (err)
+			goto out_srv;
+	} else {
+		err = EINVAL;
+		if (!port)
+	 		goto out_portarg;
+		err = ncld_gethost(&nsp->host, &nsp->port, host, port);
+		if (err)
+			goto out_srv;
+	}
+
+	nsp->event = event;
+	nsp->event_arg = ev_arg;
+
+............
+	nsp->thread = g_thread_create(ncld_sess_thr, nsp, FALSE, &gerr);
+	if (nsp->thread == NULL) {
+		HAIL_ERR(hlog, "Failed to start replication thread: %s",
+		       gerr->message);
+		err = 1002;
+		goto out_thread;
+	}
+
+	if (cldc_udp_new(nsp->host, nsp->port, &nsp->udp)) {
+		err = 1003;
+		goto out_udp;
+	}
+
+	/*
+	 * FIXME If anyone ever gets 2 sessions in one application,
+	 * their logs' lifetimes will overlap. We need to move the errlog
+	 * from ops to the session itself.
+	 */
+	ncld_ops.errlog = hlog.func;
+
+	memset(&copts, 0, sizeof(copts));
+	copts.cb = ncld_new_sess;
+	if (cldc_new_sess(&ncld_ops, &copts, nsp->udp->addr, nsp->udp->addr_len,
+			  cld_user, cld_key, nsp, &nsp->udp->sess)) {
+		err = 1004;
+		goto out_session;
+	}
+
+	/* udp->sess->log.verbose = hlog.verbose; */
+
+	wait for new session to establish
+
+	return nsp;
+
+out_session:
+	ncld_thr_poison(nsp);
+	cldc_udp_free(nsp->udp);
+out_udp:
+	ncld_thr_end(nsp);
+out_thread:
+out_portdup:
+	free(nsp->host);
+out_srv:
+out_portarg:
+	free(nsp);
+out_sesalloc:
+	*error = err;
+	return NULL;
+}
+
+/*
+ * We do not provide anything like ncld_set_callback because it inevitably
+ * leads to lost events. Unused arguments are not too onerous, so just
+ * put zero into the events if you don't want notifications.
+ *
+ * On error, return NULL and set the error code. XXX to what value?
+ */
+struct ncld_fh *ncld_open(struct ncld_sess *s, const char *fname,
+	unsigned int mode, int *error, unsigned int events,
+	void (*event)(struct ncld_fh *, void *, unsigned int), void *ev_arg)
+{
+}
+
+/*
+ * Using a read/write-type interface inevitably produces an extra data copy,
+ * but CLD is not intended as a high performance service. So, there is
+ * no get/put API, only read/write.
+ */
+long ncld_read(struct ncld_fh *, void *data, long len)
+{
+}
+
+long ncld_write(struct ncld_fh *, const void *data, long len)
+{
+}
+
+int ncld_lock(struct ncld_fh *)
+{
+}
+
+void ncld_unlock(struct ncld_fh *)
+{
+}
+
+void ncld_close(struct ncld_fh *)
+{
+}
+
+void ncld_sess_close(struct ncld_sess *nsp)
+{
+	cldc_kill_sess(nsp->udp->sess);
+	ncld_thr_poison(nsp);
+	cldc_udp_free(nsp->udp);
+	ncld_thr_end(nsp);
+	free(nsp->host);
+	free(nsp);
+}
+
+void ncld_init(void)
+{
+	cldc_init();
+}
+#endif 0 /* temporarily */
+
 /*
  * For extra safety, call cldc_init after g_thread_init, if present.
  * Currently we just call srand(), but since we use GLib, we may need

-- Pete