[Pdbg] [PATCH 07/23] libpdbg: Create virtual nodes based on device-tree-path property

[Amitay Isaacs]

On Thu, 2019-09-19 at 13:50 +1000, Alistair Popple wrote:
> On Thursday, 19 September 2019 12:33:17 PM AEST Amitay Isaacs wrote:
> > To create a virtual node, "device-tree-path" property is used to
> > denote
> > the attachment point in the system device tree view.
> 
> Can I bike shed the name? :-)
> 
> How about "system-path"?

Sure.


> 
> <snip>
> 
> > +static void pdbg_targets_init_virtual(struct pdbg_target *node,
> > struct 
> pdbg_target *root)
> > +{
> > +	struct pdbg_target *vnode, *parent, *child = NULL;
> > +	struct dt_property *prop;
> > +	const char *dtree_path;
> > +	char *parent_path, *sep;
> > +	size_t len;
> > +
> > +	dtree_path = (const char *)pdbg_target_property(node, "device-
> > tree-path", 
> &len);
> > +	if (!dtree_path)
> > +		goto skip;
> > +
> > +	vnode = dt_find_by_path(root, dtree_path);
> > +	if (vnode) {
> > +		if (vnode->vnode)
> > +			goto skip;
> > +		else
> > +			goto link_vnode;
> > +	}
> > +
> > +	parent_path = strdup(dtree_path);
> > +	assert(parent_path);
> > +
> > +	sep = strrchr(parent_path, '/');
> > +	if (!sep || sep[1] == '\0') {
> > +		char *path = dt_get_path(node);
> > +		PR_ERROR("Invalid path reference \"%s\" for node %s\n",
> > dtree_path, 
> path);
> > +		free(path);
> > +		free(parent_path);
> > +		goto skip;
> > +	}
> > +
> > +	*sep = '\0';
> > +
> > +	parent = dt_find_by_path(root, parent_path);
> > +	if (!parent) {
> > +		char *path = dt_get_path(node);
> > +		PR_ERROR("Invalid path reference \"%s\" for node %s\n",
> > dtree_path, 
> path);
> > +		free(path);
> > +		free(parent_path);
> > +		goto skip;
> > +	}
> > +
> > +	vnode = dt_new_node(sep+1, NULL, 0);
> > +	assert(vnode);
> > +
> > +	free(parent_path);
> > +
> > +	if (!dt_attach_node(parent, vnode))
> > +		goto skip;
> > +
> > +	PR_DEBUG("Created virtual node %s\n", dtree_path);
> > +
> > +link_vnode:
> > +	node->vnode = vnode;
> > +	vnode->vnode = node;
> > +
> > +	while ((prop = list_pop(&vnode->properties, struct dt_property,
> > list)))
> > +		list_add_tail(&node->properties, &prop->list);
> 
> This isn't terrible to follow, but I wonder if it couldn't also be
> refactored 
> to be a little cleaner by splitting link_vnode and vnode creation (or
> perhaps 
> just path validation/lookup) into separate functions.

Makes sense.  I will refactor a function to create a virtual node.

> 
> - Alistair
> 
> > +skip:
> > +	list_for_each(&node->children, child, list) {
> > +		if (child->vnode && !child->compatible)
> > +			continue;
> > +
> > +		pdbg_targets_init_virtual(child, root);
> > +	}
> > +}
> > +
> >  void pdbg_targets_init(void *fdt)
> >  {
> >  	/* Root node needs to be valid when this function returns */
> > @@ -663,6 +732,7 @@ void pdbg_targets_init(void *fdt)
> >  	}
> >  
> >  	dt_expand(pdbg_dt_root, fdt);
> > +	pdbg_targets_init_virtual(pdbg_dt_root, pdbg_dt_root);
> >  }
> >  
> >  char *pdbg_target_path(struct pdbg_target *target)
> > 
> 
> 
> 

Amitay.
-- 

Well done is better than well said. - Ben Franklin