diff mbox series

[04/10] path: Add device tree path based targeting

Message ID 20181002060430.3344784-5-amitay@ozlabs.org
State Superseded
Headers show
Series Device tree path base targeting | expand

Commit Message

Amitay Isaacs Oct. 2, 2018, 6:04 a.m. UTC
Signed-off-by: Amitay Isaacs <amitay@ozlabs.org>
---
 Makefile.am |   2 +
 src/path.c  | 383 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 src/path.h  | 125 +++++++++++++++++
 3 files changed, 510 insertions(+)
 create mode 100644 src/path.c
 create mode 100644 src/path.h

Comments

Alistair Popple Nov. 1, 2018, 1:56 a.m. UTC | #1
> +/**
> + * @brief Iterator for list of path targets
> + *
> + * @param[in]  prev The last pdbg target
> + * @param[in]  only_enabled Whether to list enabled targets or all

Just a minor documentation nit-pick ... reading that makes me think it would 
just check the status. However it also has the side-effect of trying to probe 
the device first.

In general I think it would be nicer to have seperate macros/calls for probed 
and enabled only vs. iterating over all rather than a bool flag whos meaning is 
a little less obvious when reading the calls in code.

- Alistair

> + * @return the next pdbg target in the list, NULL otherwise
> + *
> + * If prev is NULL, then return the first pdbg target in the list.
> + * If there are no more pdbg targets in the list, NULL is returned.
> + */
> +struct pdbg_target *path_target_next(struct pdbg_target *prev,
> +				     bool only_enabled);
> +
> +/**
> + * @brief Iterator for a list of path targets of specified class
> + *
> + * @param[in]  klass The class of the targets required
> + * @param[in]  prev The last pdbg target
> + * @param[in]  only_enabled Whether to list enabled targets or all
> + * @return the next matching pdbg target in the list, NULL otherwise
> + *
> + * If prev is NULL, then return the first matching pdbg target in the list.
> + * If there are no more matching pdbg targets, NULL is returned.
> + */
> +struct pdbg_target *path_target_next_class(const char *klass,
> +					   struct pdbg_target *prev,
> +					   bool only_enabled);
> +
> +/**
> + * @brief Macro for iterating through all path targets
> + *
> + * target is of type struct pdbg_target
> + * only_eanbled is boolean
> + *
> + * If only_enabled is true, iterate through targets that are enabled;
> + * otherwise iterate through all targets
> + */
> +#define for_each_path_target(target, only_enabled)          \
> +	for (target = path_target_next(NULL, only_enabled); \
> +	     target;                                        \
> +	     target = path_target_next(target, only_enabled))
> +
> +/**
> + * @brief Macro for iterating through all path targets of specific class
> + *
> + * class is of type char *
> + * target is of type struct pdbg_target
> + * only_eanbled is boolean
> + *
> + * If only_enabled is true, iterate through targets that are enabled;
> + * otherwise iterate through all targets
> + */
> +#define for_each_path_target_class(class, target, only_enabled)          \
> +	for (target = path_target_next_class(class, NULL, only_enabled); \
> +	     target;                                                     \
> +	     target = path_target_next_class(class, target, only_enabled))
> +
> +#endif
Amitay Isaacs Nov. 1, 2018, 3:06 a.m. UTC | #2
On Thu, 2018-11-01 at 12:56 +1100, Alistair Popple wrote:
> > +/**
> > + * @brief Iterator for list of path targets
> > + *
> > + * @param[in]  prev The last pdbg target
> > + * @param[in]  only_enabled Whether to list enabled targets or all
> 
> Just a minor documentation nit-pick ... reading that makes me think
> it would 
> just check the status. However it also has the side-effect of trying
> to probe 
> the device first.

Hmm..  I added probing in line with the current code.  But now I think
that this code should not probe targets at all.  That way there are no
side-effects.

I think we should add explicit probing before running the command and
explicit release after the command has executed.  That way you don't
need incremental probing as done in the for_each_target and
for_each_child_target macros.

How about something like:

   for_each_path_target(target)
      pdbg_target_probe(target);

   rc = cmd(args, flags);

   for_each_path_target(target)
      pdbg_target_release(target); 

Then each command does not have to worry about the probing targets and
only use for_each_path_target_enabled() or
for_each_path_target_class().  May be we will also need
for_each_path_target_class_enabled().

> In general I think it would be nicer to have seperate macros/calls
> for probed 
> and enabled only vs. iterating over all rather than a bool flag whos
> meaning is 
> a little less obvious when reading the calls in code.

Sure. 

Amitay.
Alistair Popple Nov. 1, 2018, 11:13 p.m. UTC | #3
On Thursday, 1 November 2018 2:06:50 PM AEDT Amitay Isaacs wrote:
> On Thu, 2018-11-01 at 12:56 +1100, Alistair Popple wrote:
> > > +/**
> > > + * @brief Iterator for list of path targets
> > > + *
> > > + * @param[in]  prev The last pdbg target
> > > + * @param[in]  only_enabled Whether to list enabled targets or all
> > 
> > Just a minor documentation nit-pick ... reading that makes me think
> > it would
> > just check the status. However it also has the side-effect of trying
> > to probe
> > the device first.
> 
> Hmm..  I added probing in line with the current code.  But now I think
> that this code should not probe targets at all.  That way there are no
> side-effects.
> 
> I think we should add explicit probing before running the command and
> explicit release after the command has executed.  That way you don't
> need incremental probing as done in the for_each_target and
> for_each_child_target macros.
> 
> How about something like:
> 
>    for_each_path_target(target)
>       pdbg_target_probe(target);
> 
>    rc = cmd(args, flags);
> 
>    for_each_path_target(target)
>       pdbg_target_release(target);
> 
> Then each command does not have to worry about the probing targets and
> only use for_each_path_target_enabled() or
> for_each_path_target_class().  May be we will also need
> for_each_path_target_class_enabled().

Yep. I think that makes more sense. From memory you only use 
only_enabled==true in one to two locations so hopefully changing it won't 
cause too much churn.

- Alistair

> > In general I think it would be nicer to have seperate macros/calls
> > for probed
> > and enabled only vs. iterating over all rather than a bool flag whos
> > meaning is
> > a little less obvious when reading the calls in code.
> 
> Sure.
> 
> Amitay.
diff mbox series

Patch

diff --git a/Makefile.am b/Makefile.am
index 8bf2ba0..653b7d9 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -78,6 +78,8 @@  pdbg_SOURCES = \
 	src/options.h \
 	src/parsers.c \
 	src/parsers.h \
+	src/path.c \
+	src/path.h \
 	src/progress.c \
 	src/progress.h \
 	src/reg.c \
diff --git a/src/path.c b/src/path.c
new file mode 100644
index 0000000..66098bc
--- /dev/null
+++ b/src/path.c
@@ -0,0 +1,383 @@ 
+/* Copyright 2018 IBM Corp.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * 	http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
+ * implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+#include <stdio.h>
+#include <string.h>
+#include <stdbool.h>
+#include <stdlib.h>
+#include <ctype.h>
+#include <assert.h>
+#include <limits.h>
+
+#include <libpdbg.h>
+
+#include "path.h"
+#include "util.h"
+
+#define MAX_PATH_COMP_LEN	32
+#define MAX_PATH_COMPONENTS	16
+#define MAX_PATH_LEN		(MAX_PATH_COMP_LEN * MAX_PATH_COMPONENTS)
+
+/* This is max(MAX_PROCESSORS, MAX_CHIPS, MAX_THREADS) */
+#define MAX_PATH_INDEX		64
+
+struct path_pattern {
+	char prefix[MAX_PATH_COMP_LEN];
+	int index[MAX_PATH_INDEX];
+	bool match_full;
+	bool match_index;
+};
+
+/* Start with max threads, rather than defining arbitrary number */
+#define MAX_TARGETS	(64 * 24 * 8)
+
+static struct pdbg_target *path_target[MAX_TARGETS];
+static unsigned int path_target_count;
+
+static void safe_strcpy(char *dest, size_t n, const char *src)
+{
+	assert(strlen(src) + 1 <= n);
+
+	strcpy(dest, src);
+}
+
+static void safe_strcat(char *dest, size_t n, const char *src)
+{
+	assert(strlen(dest) + strlen(src) + 1 <= n);
+
+	strcat(dest, src);
+}
+
+/*
+ * Parse string components of following forms:
+ *	pib0
+ *	core[1-3,11-13]
+ *	thread*
+ *	adu@123000
+ */
+static bool path_pattern_parse(const char *arg, struct path_pattern *pat)
+{
+	char tmp[strlen(arg)+1];
+	char *tok;
+	bool ok;
+
+	safe_strcpy(tmp, sizeof(tmp), arg);
+
+	memset(pat, 0, sizeof(*pat));
+
+	if (strchr(tmp, '@')) {
+		safe_strcpy(pat->prefix, sizeof(pat->prefix), tmp);
+		pat->match_full = true;
+
+	} else if (strchr(tmp, '*')) {
+		tok = strtok(tmp, "*");
+		if (!tok)
+			safe_strcpy(pat->prefix, sizeof(pat->prefix), "all");
+		else
+			safe_strcpy(pat->prefix, sizeof(pat->prefix), tok);
+
+	} else if (strchr(tmp, '[')) {
+		tok = strtok(tmp, "[");
+		if (tok == NULL) {
+			fprintf(stderr, "Invalid pattern '%s'\n", arg);
+			return false;
+		}
+		safe_strcpy(pat->prefix, sizeof(pat->prefix), tok);
+
+		tok = strtok(NULL, "]");
+		if (!tok) {
+			fprintf(stderr, "Invalid pattern '%s'\n", arg);
+			return false;
+		}
+
+		ok = parse_list(tok, MAX_PATH_INDEX, pat->index, NULL);
+		if (!ok)
+			return false;
+
+		pat->match_index = true;
+
+	} else {
+		size_t n = strlen(tmp) - 1;
+		while (n >= 0 && isdigit(tmp[n]))
+			n--;
+		n++;
+
+		if (n != strlen(tmp)) {
+			int index;
+
+			index = atoi(&tmp[n]);
+			if (index < 0 || index >= MAX_PATH_INDEX) {
+				fprintf(stderr, "Invalid index '%s'\n", &tmp[n]);
+				return false;
+			}
+			tmp[n] = '\0';
+			pat->index[index] = 1;
+			pat->match_index = true;
+		}
+
+		safe_strcpy(pat->prefix, sizeof(pat->prefix), tmp);
+	}
+
+	if (!pat->prefix)
+		return false;
+
+	return true;
+}
+
+static int path_pattern_split(const char *arg, struct path_pattern *pats)
+{
+	char arg_copy[MAX_PATH_LEN];
+	char *tmp, *tok, *saveptr = NULL;
+	int n;
+	bool ok;
+
+	safe_strcpy(arg_copy, sizeof(arg_copy), arg);
+
+	tmp = arg_copy;
+	n = 0;
+	while ((tok = strtok_r(tmp, "/", &saveptr))) {
+		size_t len = strlen(tok);
+
+		if (len == 0)
+			continue;
+
+		if (len >= MAX_PATH_LEN) {
+			fprintf(stderr, "Pattern component '%s' too long\n", tok);
+			return -1;
+		}
+
+		ok = path_pattern_parse(tok, &pats[n]);
+		if (!ok)
+			return -1;
+
+		n++;
+		assert(n <= MAX_PATH_COMPONENTS);
+
+		tmp = NULL;
+	}
+
+	return n;
+}
+
+static int path_target_find(struct pdbg_target *prev)
+{
+	int i;
+
+	if (!prev)
+		return -1;
+
+	for (i=0; i<path_target_count; i++) {
+		if (path_target[i] == prev)
+			return i;
+	}
+
+	return -1;
+}
+
+struct pdbg_target *path_target_find_next(const char *klass,
+					  int index,
+					  bool only_enabled)
+{
+	struct pdbg_target *target;
+	enum pdbg_target_status status;
+	int i;
+
+	for (i=index+1; i<path_target_count; i++) {
+		target = path_target[i];
+		if (only_enabled) {
+			pdbg_target_probe(target);
+			status = pdbg_target_status(target);
+			if (status != PDBG_TARGET_ENABLED)
+				continue;
+		}
+		if (klass) {
+			const char *classname = pdbg_target_class_name(target);
+
+			if (!strcmp(klass, classname))
+				return target;
+		} else {
+			return target;
+		}
+	}
+
+	return NULL;
+}
+
+bool path_target_add(struct pdbg_target *target)
+{
+	int index;
+
+	index = path_target_find(target);
+	if (index >= 0)
+		return true;
+
+	if (path_target_count == MAX_TARGETS)
+		return false;
+
+	path_target[path_target_count] = target;
+	path_target_count++;
+	return true;
+}
+
+static void path_pattern_match(struct pdbg_target *target,
+			       struct path_pattern *pats,
+			       int max_levels,
+			       int level)
+{
+	struct pdbg_target *child;
+	char dn_name[MAX_PATH_COMP_LEN];
+	char *tok;
+	int next = level;
+	bool found = false;
+
+	if (target == pdbg_target_root()) {
+		goto end;
+	}
+
+	if (!strcmp("all", pats[level].prefix)) {
+		if (!path_target_add(target))
+			return;
+		goto end;
+	}
+
+	safe_strcpy(dn_name, sizeof(dn_name), pdbg_target_dn_name(target));
+	if (pats[level].match_full) {
+		tok = dn_name;
+	} else {
+		tok = strtok(dn_name, "@");
+	}
+
+	if (!strcmp(tok, pats[level].prefix)) {
+		found = true;
+
+		if (pats[level].match_index) {
+			int index = pdbg_target_index(target);
+
+			if (pats[level].index[index] != 1)
+				found = false;
+		}
+	}
+
+	if (found) {
+		if (level == max_levels-1) {
+			path_target_add(target);
+			return;
+		} else {
+			next = level + 1;
+		}
+	}
+
+end:
+	pdbg_for_each_child_target(target, child) {
+		path_pattern_match(child, pats, max_levels, next);
+	}
+}
+
+bool path_target_parse(const char **arg, int arg_count)
+{
+	struct path_pattern pats[MAX_PATH_COMPONENTS];
+	int i, n;
+
+	for (i=0; i<arg_count; i++) {
+		n = path_pattern_split(arg[i], pats);
+		if (n <= 0)
+			return false;
+
+		path_pattern_match(pdbg_target_root(), pats, n, 0);
+	}
+
+	return true;
+}
+
+bool path_target_present(void)
+{
+	return (path_target_count > 0);
+}
+
+bool path_target_selected(struct pdbg_target *target)
+{
+	int index;
+
+	index = path_target_find(target);
+	if (index >= 0)
+		return true;
+
+	return false;
+}
+
+static void path_target_build_path(struct pdbg_target *target, char *path, size_t len)
+{
+	char dn_name[MAX_PATH_LEN], *tok;
+	char number[16];
+	int index;
+
+	if (target != pdbg_target_root()) {
+		path_target_build_path(pdbg_target_parent(NULL, target), path, len);
+		index = pdbg_target_index(target);
+		if (index != -1) {
+			safe_strcpy(dn_name, sizeof(dn_name), pdbg_target_dn_name(target));
+			tok = strtok(dn_name, "@");
+			sprintf(number, "%d", index);
+			safe_strcat(path, len, "/");
+			safe_strcat(path, len, tok);
+			safe_strcat(path, len, number);
+		} else {
+			safe_strcat(path, len, "/");
+			safe_strcat(path, len, pdbg_target_dn_name(target));
+		}
+	} else {
+		safe_strcpy(path, len, "");
+	}
+}
+
+char *path_target_path(struct pdbg_target *target)
+{
+	char path[MAX_PATH_LEN];
+
+	path_target_build_path(target, path, sizeof(path));
+	return strdup(path);
+}
+
+void path_target_dump(void)
+{
+	struct pdbg_target *target;
+	char *path;
+
+	for_each_path_target(target, false) {
+		path = path_target_path(target);
+		assert(path);
+		printf("%s\n", path);
+		free(path);
+	}
+}
+
+struct pdbg_target *path_target_next(struct pdbg_target *prev,
+				     bool only_enabled)
+{
+	int index;
+
+	index = path_target_find(prev);
+	return path_target_find_next(NULL, index, only_enabled);
+}
+
+struct pdbg_target *path_target_next_class(const char *klass,
+					   struct pdbg_target *prev,
+					   bool only_enabled)
+{
+	int index;
+
+	index = path_target_find(prev);
+	return path_target_find_next(klass, index, only_enabled);
+}
diff --git a/src/path.h b/src/path.h
new file mode 100644
index 0000000..35c3a6e
--- /dev/null
+++ b/src/path.h
@@ -0,0 +1,125 @@ 
+/* Copyright 2018 IBM Corp.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * 	http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
+ * implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+#ifndef __PDBG_PATH_H
+#define __PDBG_PATH_H
+
+#include <libpdbg.h>
+
+/**
+ * @brief Parse a list of path target strings
+ *
+ * @param[in]  arg An array of strings
+ * @param[in]  arg_count The number of strings
+ * @return true on success, false on error
+ */
+bool path_target_parse(const char **arg, int arg_count);
+
+/**
+ * @brief Check if there are any path targets
+ *
+ * @return true if number of path targets > 0, false otherwise
+ */
+bool path_target_present(void);
+
+/**
+ * @brief Add a target to the list
+ *
+ * @param[in]  target pdbg target
+ * @return true on success, false on error
+ */
+bool path_target_add(struct pdbg_target *target);
+
+/**
+ * @brief Check if the specified target is selected
+ *
+ * @param[in]  target pdbg target
+ * @return true if the target is selected, false otherwise
+ */
+bool path_target_selected(struct pdbg_target *target);
+
+/**
+ * @brief Get the device-tree path for a target
+ *
+ * @param[in]  target pdbg target
+ * @return path for the target, NULL on error
+ *
+ * The path is allocated by malloc and should be freed by caller.
+ */
+char *path_target_path(struct pdbg_target *target);
+
+/**
+ * @brief Print the list of path targets to stdout
+ */
+void path_target_dump(void);
+
+/**
+ * @brief Iterator for list of path targets
+ *
+ * @param[in]  prev The last pdbg target
+ * @param[in]  only_enabled Whether to list enabled targets or all
+ * @return the next pdbg target in the list, NULL otherwise
+ *
+ * If prev is NULL, then return the first pdbg target in the list.
+ * If there are no more pdbg targets in the list, NULL is returned.
+ */
+struct pdbg_target *path_target_next(struct pdbg_target *prev,
+				     bool only_enabled);
+
+/**
+ * @brief Iterator for a list of path targets of specified class
+ *
+ * @param[in]  klass The class of the targets required
+ * @param[in]  prev The last pdbg target
+ * @param[in]  only_enabled Whether to list enabled targets or all
+ * @return the next matching pdbg target in the list, NULL otherwise
+ *
+ * If prev is NULL, then return the first matching pdbg target in the list.
+ * If there are no more matching pdbg targets, NULL is returned.
+ */
+struct pdbg_target *path_target_next_class(const char *klass,
+					   struct pdbg_target *prev,
+					   bool only_enabled);
+
+/**
+ * @brief Macro for iterating through all path targets
+ *
+ * target is of type struct pdbg_target
+ * only_eanbled is boolean
+ *
+ * If only_enabled is true, iterate through targets that are enabled;
+ * otherwise iterate through all targets
+ */
+#define for_each_path_target(target, only_enabled)          \
+	for (target = path_target_next(NULL, only_enabled); \
+	     target;                                        \
+	     target = path_target_next(target, only_enabled))
+
+/**
+ * @brief Macro for iterating through all path targets of specific class
+ *
+ * class is of type char *
+ * target is of type struct pdbg_target
+ * only_eanbled is boolean
+ *
+ * If only_enabled is true, iterate through targets that are enabled;
+ * otherwise iterate through all targets
+ */
+#define for_each_path_target_class(class, target, only_enabled)          \
+	for (target = path_target_next_class(class, NULL, only_enabled); \
+	     target;                                                     \
+	     target = path_target_next_class(class, target, only_enabled))
+
+#endif