Remove instances of TODO and if 0dev-dct-lvmlockd-AP

9 files changed, 46 insertions, 159 deletions

diff --git a/daemons/lvmlockd/lvmlockctl.c b/daemons/lvmlockd/lvmlockctl.c
index 17b122f9d..b8ab9ed11 100644
--- a/daemons/lvmlockd/lvmlockctl.c
+++ b/daemons/lvmlockd/lvmlockctl.c
@@ -311,6 +311,9 @@ static daemon_reply _lvmlockd_send(const char *req_name, ...)
 	return repl;
 }
 
+/* See the same in lib/locking/lvmlockd.c */
+#define NO_LOCKD_RESULT -1000
+
 static int _lvmlockd_result(daemon_reply reply, int *result)
 {
 	int reply_result;
@@ -327,9 +330,7 @@ static int _lvmlockd_result(daemon_reply reply, int *result)
 		return 0;
 	}
 
-	/* FIXME: using -1000 is dumb */
-
-	reply_result = daemon_reply_int(reply, "op_result", -1000);
+	reply_result = daemon_reply_int(reply, "op_result", NO_LOCKD_RESULT);
 	if (reply_result == -1000) {
 		log_error("lvmlockd_result no op_result");
 		return 0;
diff --git a/daemons/lvmlockd/lvmlockd-core.c b/daemons/lvmlockd/lvmlockd-core.c
index 7c962c8eb..cd48a90d9 100644
--- a/daemons/lvmlockd/lvmlockd-core.c
+++ b/daemons/lvmlockd/lvmlockd-core.c
@@ -1620,7 +1620,7 @@ static void res_process(struct lockspace *ls, struct resource *r,
 			add_client_result(act);
 		} else {
 			/* persistent lock is sh, transient request is ex */
-			/* TODO: can we remove this case? do a convert here? */
+			/* FIXME: can we remove this case? do a convert here? */
 			log_debug("res_process %s existing persistent lock new transient", r->name);
 			act->result = -EEXIST;
 			list_del(&act->list);
@@ -1654,7 +1654,7 @@ static void res_process(struct lockspace *ls, struct resource *r,
 			continue;
 
 		if (lk->mode != act->mode) {
-			/* TODO: convert and change to persistent? */
+			/* FIXME: convert and change to persistent? */
 			log_debug("res_process %s existing transient lock new persistent", r->name);
 			act->result = -EEXIST;
 			list_del(&act->list);
@@ -2342,7 +2342,7 @@ static int vg_ls_name(const char *vg_name, char *ls_name)
 	return 0;
 }
 
-/* TODO: add mutex for gl_lsname_ ? */
+/* FIXME: add mutex for gl_lsname_ ? */
 
 static int gl_ls_name(char *ls_name)
 {
@@ -2811,10 +2811,7 @@ static int for_each_lockspace(int do_stop, int do_free, int do_force)
 				pthread_join(ls->thread, NULL);
 				list_del(&ls->list);
 
-				/* TODO: remove this if unneeded */
-				if (!list_empty(&ls->actions))
-					log_error("TODO: free ls actions");
-
+				/* In future we may need to free ls->actions here */
 				free_ls_resources(ls);
 				list_add(&ls->list, &lockspaces_inactive);
 				free_count++;
@@ -4031,7 +4028,7 @@ static void client_recv_action(struct client *cl)
 	    op == LD_OP_DUMP_INFO || op == LD_OP_DUMP_LOG) {
 
 		/*
-		 * TODO: add the client command name to the hello messages
+		 * FIXME: add the client command name to the hello messages
 		 * so it can be saved in cl->name here.
 		 */
 
@@ -4104,7 +4101,7 @@ static void client_recv_action(struct client *cl)
 	if (cl_pid && cl_pid != cl->pid)
 		log_error("client recv bad message pid %d client %d", cl_pid, cl->pid);
 
-	/* TODO: do this in hello message instead */
+	/* FIXME: do this in hello message instead */
 	if (!cl->name[0] && cl_name)
 		strncpy(cl->name, cl_name, MAX_NAME);
 
@@ -4343,52 +4340,6 @@ static void close_client_thread(void)
 	pthread_join(client_thread, NULL);
 }
 
-#if 0
-static void setup_listener(void)
-{
-	struct sockaddr_un addr;
-	int rv, fd, ci;
-
-	rv = lvmlockd_socket_address(&addr);
-	if (rv < 0)
-		return rv;
-
-	fd = socket(AF_LOCAL, SOCK_STREAM, 0);
-	if (fd < 0)
-		return fd;
-
-	unlink(addr.sun_path);
-	rv = bind(fd, (struct sockaddr *) &addr, sizeof(struct sockaddr_un));
-	if (rv < 0)
-		goto exit_fail;
-
-	rv = chmod(addr.sun_path, DEFAULT_SOCKET_MODE);
-	if (rv < 0)
-		goto exit_fail;
-
-	rv = chown(addr.sun_path, com.uid, com.gid);
-	if (rv < 0) {
-		log_error("could not set socket %s permissions: %s",
-			  addr.sun_path, strerror(errno));
-		goto exit_fail;
-	}
-
-	rv = listen(fd, 5);
-	if (rv < 0)
-		goto exit_fail;
-
-	fcntl(fd, F_SETFL, fcntl(fd, F_GETFL, 0) | O_NONBLOCK);
-
-	listen_pi = add_pollfd(fd);
-
-	return 0;
-
-exit_fail:
-	close(fd);
-	return -1;
-}
-#endif
-
 /*
  * Get a list of all VGs with a lockd type (sanlock|dlm) from lvmetad.
  * We'll match this list against a list of existing lockspaces that are
@@ -4929,7 +4880,7 @@ static void adopt_locks(void)
 	list_for_each_entry_safe(ls, lsafe, &vg_lockd, list) {
 		if (!list_empty(&ls->resources)) {
 			/* We should have found a lockspace. */
-			/* TODO: add this ls and acquire locks for ls->resources? */
+			/* add this ls and acquire locks for ls->resources? */
 			log_error("No lockspace %s %s found for VG %s with active LVs",
 				  ls->name, lm_str(ls->lm_type), ls->vg_name);
 		} else {
@@ -5249,7 +5200,7 @@ static void adopt_locks(void)
 
 		if (act->mode == LD_LK_EX) {
 			/*
-			 * TODO: we probably want to check somehow that
+			 * FIXME: we probably want to check somehow that
 			 * there's no lvm command still running that's
 			 * using this ex lock and changing things.
 			 */
@@ -5294,7 +5245,7 @@ static void adopt_locks(void)
 	}
 
 
-	/* TODO: purge any remaining orphan locks in each rejoined ls? */
+	/* FIXME: purge any remaining orphan locks in each rejoined ls? */
 
 	if (count_start_fail || count_adopt_fail)
 		goto fail;
@@ -5546,8 +5497,8 @@ static int main_loop(daemon_state *ds_arg)
 			}
 			pthread_mutex_unlock(&client_mutex);
 
-			/* TODO?: after set_dead, scan pollfd for last unused
-			   slot and reduce pollfd_maxi */
+			/* After set_dead, should we scan pollfd for
+			   last unused slot and reduce pollfd_maxi? */
 		}
 	}
 
diff --git a/daemons/lvmlockd/lvmlockd-dlm.c b/daemons/lvmlockd/lvmlockd-dlm.c
index 04336fd66..96b5b2be0 100644
--- a/daemons/lvmlockd/lvmlockd-dlm.c
+++ b/daemons/lvmlockd/lvmlockd-dlm.c
@@ -236,10 +236,11 @@ int lm_rem_lockspace_dlm(struct lockspace *ls, int free_vg)
 		goto out;
 
 	/*
-	 * TODO: if free_vg is set, it means we are doing vgremove,
-	 * and we may want to tell any other nodes to leave the lockspace.
-	 * This is not really necessary since there should be no harm in
-	 * having an unused lockspace sitting around.
+	 * If free_vg is set, it means we are doing vgremove, and we may want
+	 * to tell any other nodes to leave the lockspace.  This is not really
+	 * necessary since there should be no harm in having an unused
+	 * lockspace sitting around.  A new "notification lock" would need to
+	 * be added with a callback to signal this. 
 	 */
 
 	rv = dlm_release_lockspace(ls->name, lmd->dh, 1);
@@ -403,7 +404,7 @@ static int lm_adopt_dlm(struct lockspace *ls, struct resource *r, int ld_mode,
 	}
 
 	/*
-	 * TODO: for GL/VG locks we probably want to read the lvb,
+	 * FIXME: For GL/VG locks we probably want to read the lvb,
 	 * especially if adopting an ex lock, because when we
 	 * release this adopted ex lock we may want to write new
 	 * lvb values based on the current lvb values (at lease
@@ -478,7 +479,6 @@ int lm_lock_dlm(struct lockspace *ls, struct resource *r, int ld_mode,
 			      r->name, strlen(r->name),
 			      0, NULL, NULL, NULL);
 	if (rv == -EAGAIN) {
-		/* TODO: what case is this? what should be done? */
 		log_error("S %s R %s lock_dlm mode %d rv EAGAIN", ls->name, r->name, mode);
 		return -EAGAIN;
 	}
@@ -557,7 +557,7 @@ int lm_convert_dlm(struct lockspace *ls, struct resource *r,
 			      r->name, strlen(r->name),
 			      0, NULL, NULL, NULL);
 	if (rv == -EAGAIN) {
-		/* TODO: what case is this? what should be done? */
+		/* FIXME: When does this happen?  Should something different be done? */
 		log_error("S %s R %s convert_dlm mode %d rv EAGAIN", ls->name, r->name, mode);
 		return -EAGAIN;
 	}
diff --git a/daemons/lvmlockd/lvmlockd-internal.h b/daemons/lvmlockd/lvmlockd-internal.h
index f5f7a399c..39085a6fc 100644
--- a/daemons/lvmlockd/lvmlockd-internal.h
+++ b/daemons/lvmlockd/lvmlockd-internal.h
@@ -11,7 +11,6 @@
 #ifndef _LVM_LVMLOCKD_INTERNAL_H
 #define _LVM_LVMLOCKD_INTERNAL_H
 
-/* TODO: figure out real restraints/requirements for these */
 #define MAX_NAME 64
 #define MAX_ARGS 64
 
@@ -184,7 +183,6 @@ struct lockspace {
 
 	struct list_head actions;	/* new client actions */
 	struct list_head resources;	/* resource/lock state for gl/vg/lv */
-					/* TODO: should probably be tree */
 };
 
 #define VAL_BLK_VERSION 0x0101
diff --git a/daemons/lvmlockd/lvmlockd-sanlock.c b/daemons/lvmlockd/lvmlockd-sanlock.c
index fa7ad8ab2..85a52c880 100644
--- a/daemons/lvmlockd/lvmlockd-sanlock.c
+++ b/daemons/lvmlockd/lvmlockd-sanlock.c
@@ -82,8 +82,8 @@
  * the error conditions are often not as simple as storage being lost and then
  * later connecting, will result in this option being too unreliable.
  *
- * TODO: add a config option that we could use to select a different behavior
- * than the default.  Then implement one of the simpler options as a proof of
+ * Add a config option that we could use to select a different behavior than
+ * the default.  Then implement one of the simpler options as a proof of
  * concept, which could be extended if needed.
  */
 
@@ -563,7 +563,7 @@ int lm_rename_vg_sanlock(char *ls_name, char *vg_name, uint32_t flags, char *vg_
 	if (daemon_test)
 		return 0;
 
-	/* FIXME: remove this, device is not always ready for us here */
+	/* FIXME: device is not always ready for us here */
 	sleep(1);
 
 	align_size = sanlock_align(&disk);
@@ -997,13 +997,13 @@ int lm_prepare_lockspace_sanlock(struct lockspace *ls)
 	 * activated before lvmlockd is asked to add the lockspace.
 	 * (sanlock needs to use the lv.)
 	 *
-	 * TODO: can we ask something on the system to activate the
-	 * sanlock lv or should we just require that vgchange be used
-	 * to start sanlock vgs?
-	 * Should sanlock lvs be "auto-activated"?
+	 * In the future we might be able to ask something on the system
+	 * to activate the sanlock lv from here, and with that we might be
+	 * able to start sanlock VGs without requiring a
+	 * vgchange --lock-start command.
 	 */
 
-	/* FIXME: remove this, device is not always ready for us here */
+	/* FIXME: device is not always ready for us here */
 	sleep(1);
 
 	rv = stat(disk_path, &st);
@@ -1186,7 +1186,7 @@ out:
 	free(lms);
 	ls->lm_data = NULL;
 
-	/* TODO: should we only clear gl_lsname when doing free_vg? */
+	/* FIXME: should we only clear gl_lsname when doing free_vg? */
 
 	if (!strcmp(ls->name, gl_lsname_sanlock))
 		memset(gl_lsname_sanlock, 0, sizeof(gl_lsname_sanlock));
@@ -1194,62 +1194,6 @@ out:
 	return 0;
 }
 
-#if 0
-static int find_lv_offset(struct lockspace *ls, struct resource *r,
-			  uint64_t *lv_args_offset)
-{
-	struct lm_sanlock *lms = (struct lm_sanlock *)ls->lm_data;
-	struct sanlk_resourced rd;
-	uint64_t offset;
-	int align_size;
-	int lv_count = 0;
-	int rv;
-
-	memset(&rd, 0, sizeof(rd));
-
-	strncpy(rd.rs.lockspace_name, ls->name, SANLK_NAME_LEN);
-	rd.rs.num_disks = 1;
-	memcpy(rd.rs.disks[0].path, lms->ss.host_id_disk.path, SANLK_PATH_LEN);
-
-	align_size = sanlock_align(&rd.rs.disks[0]);
-	if (align_size <= 0) {
-		log_error("find_lv_offset align error %d", align_size);
-		return -EINVAL;
-	}
-
-	offset = align_size * LV_LOCK_BEGIN;
-
-	while (1) {
-		rd.rs.disks[0].offset = offset;
-
-		memset(rd.rs.name, 0, SANLK_NAME_LEN);
-
-		rv = sanlock_read_resource(&rd.rs, 0);
-		if (rv == -EMSGSIZE || rv == -ENOSPCE) {
-			/* This indicates the end of the device is reached. */
-			rv = -EMSGSIZE;
-			break;
-		}
-
-		if (rv) {
-			log_error("S %s find_lv_offset read error %d offset %llu",
-				  ls->name, rv, (unsigned long long)offset);
-			break;
-		}
-
-		if (!strncmp(rd.rs.name, r->name, SANLK_NAME_LEN)) {
-			/* found it */
-			*lv_args_offset = offset;
-			rv = 0;
-			break;
-		}
-
-		offset += align_size;
-	}
-	return rv;
-}
-#endif
-
 static int lm_add_resource_sanlock(struct lockspace *ls, struct resource *r)
 {
 	struct lm_sanlock *lms = (struct lm_sanlock *)ls->lm_data;
@@ -1281,7 +1225,7 @@ int lm_rem_resource_sanlock(struct lockspace *ls, struct resource *r)
 {
 	struct rd_sanlock *rds = (struct rd_sanlock *)r->lm_data;
 
-	/* TODO: assert r->mode == UN or unlock if it's not? */
+	/* FIXME: assert r->mode == UN or unlock if it's not? */
 
 	if (rds->vb)
 		free(rds->vb);
@@ -1384,8 +1328,6 @@ int lm_lock_sanlock(struct lockspace *ls, struct resource *r, int ld_mode,
 		 * It appears that sanlock_acquire returns EAGAIN when we request
 		 * a shared lock but the lock is held ex by another host.
 		 * There's no point in retrying this case, just return an error.
-		 *
-		 * TODO: verify the sanlock behavior here.
 		 */
 		log_debug("S %s R %s lock_san acquire mode %d rv EAGAIN", ls->name, r->name, ld_mode);
 		*retry = 0;
@@ -1443,8 +1385,6 @@ int lm_lock_sanlock(struct lockspace *ls, struct resource *r, int ld_mode,
 		 * We can't distinguish between the two cases above,
 		 * so if requesting a sh lock, retry a couple times,
 		 * otherwise don't.
-		 *
-		 * TODO: verify sanlock behavior here.
 		 */
 		log_debug("S %s R %s lock_san acquire mode %d rv %d", ls->name, r->name, ld_mode, rv);
 		*retry = (ld_mode == LD_LK_SH) ? 1 : 0;
@@ -1548,7 +1488,7 @@ int lm_convert_sanlock(struct lockspace *ls, struct resource *r,
 
 	rv = sanlock_convert(lms->sock, -1, flags, rs);
 	if (rv == -EAGAIN) {
-		/* TODO: what case is this? what should be done? */
+		/* FIXME: When could this happen?  Should something different be done? */
 		log_error("S %s R %s convert_san EAGAIN", ls->name, r->name);
 		return -EAGAIN;
 	}
@@ -1703,19 +1643,16 @@ int lm_hosts_sanlock(struct lockspace *ls, int notify)
 	free(hss);
 
 	if (found_others && notify) {
-#if 0
-		struct sanlk_host_event he;
-		memset(&he, 0, sizeof(he));
-		hm.host_id = 1;
-		hm.generation = 0;
-		hm.event = EVENT_VGSTOP;
-		sanlock_set_event(ls->name, &he, SANLK_SETEV_ALL_HOSTS);
-#endif
 		/*
-		 * We'll need to retry for a while before all the hosts see
-		 * this event and stop the vg.
-		 * We'll need to register for events from the lockspace
-		 * and add the registered fd to our poll set.
+		 * We could use the sanlock event mechanism to notify lvmlockd
+		 * on other hosts to stop this VG.  lvmlockd would need to
+		 * register for and listen for sanlock events in the main loop.
+		 * The events are slow to propagate.  We'd need to retry for a
+		 * while before all the hosts see the event and stop the VG.
+		 * sanlock_set_event(ls->name, &he, SANLK_SETEV_ALL_HOSTS);
+		 *
+		 * Wait to try this until there appears to be real value/interest
+		 * in doing it.
 		 */
 	}
 
diff --git a/man/lvmlockd.8.in b/man/lvmlockd.8.in
index 6e80243a0..79a3218fe 100644
--- a/man/lvmlockd.8.in
+++ b/man/lvmlockd.8.in
@@ -545,7 +545,7 @@ locks if the PV holding the locks is lost.  Contact the LVM group for
 help with this process.
 
 
-.\" TODO: This is not clean or safe enough to suggest using without help.
+.\" This is not clean or safe enough to suggest using without help.
 .\"
 .\" .SS recover from lost PV holding sanlock locks
 .\"
diff --git a/tools/lvchange.c b/tools/lvchange.c
index 05805b7be..994336934 100644
--- a/tools/lvchange.c
+++ b/tools/lvchange.c
@@ -994,7 +994,7 @@ static int _lvchange_single(struct cmd_context *cmd, struct logical_volume *lv,
 		 * Otherwise, the lv lock will be taken as non-persistent
 		 * and released when this command exits.
 		 *
-		 * TODO: use "sh" if the options imply that the lvchange
+		 * FIXME: use "sh" if the options imply that the lvchange
 		 * operation does not modify the LV.
 		 */
 		if (!lockd_lv(cmd, lv, "ex", 0)) {
diff --git a/tools/polldaemon.c b/tools/polldaemon.c
index 2ae4331b2..0544542b6 100644
--- a/tools/polldaemon.c
+++ b/tools/polldaemon.c
@@ -372,7 +372,7 @@ static int report_progress(struct cmd_context *cmd, struct poll_operation_id *id
 	int ret;
 
 	/*
-	 * TODO: we don't really need to take the vg lock here,
+	 * FIXME: we don't really need to take the vg lock here,
 	 * because we only report the progress on the same host
 	 * where the pvmove/lvconvert is happening.  This means
 	 * that the local pvmove/lvconvert/lvpoll commands are
diff --git a/tools/vgchange.c b/tools/vgchange.c
index 4e2be3452..87954cd29 100644
--- a/tools/vgchange.c
+++ b/tools/vgchange.c
@@ -645,7 +645,7 @@ static int _vgchange_locktype(struct cmd_context *cmd,
 	 * lockd_free_vg_after();
 	 */
 	if (is_lockd_type(vg->lock_type)) {
-		/* TODO */
+		/* FIXME: implement full undoing of the lock_type */
 		log_error("Changing VG %s from lock type %s not yet allowed.",
 			  vg->name, vg->lock_type);
 		return 0;