[Pdbg] [PATCH] libpdbg: Make pdbg_target_path() return a const string

[Alistair Popple]

Argh, yeah. I had some changes to detect if something is scommable which 
aren't quite ready but this was based on.

- Alistair

On Tuesday, 14 January 2020 4:15:11 PM AEDT Amitay Isaacs wrote:
> Patch does not apply to master.  Is it based on some other changes?
> 
> Amitay.
> 
> On Fri, 2020-01-10 at 15:34 +1100, Alistair Popple wrote:
> > Currently pdbg_target_path() allocates memory to return the path
> > into. As the target path is a convient indentifier when printing
> > information to user about a specific target it leads either to memory
> > leaks when called directly as part of a printf statement. The
> > alternative is lots of repeated code like the below, noting that the
> > assert can never be hit as there is already one in dt_get_path():
> > 
> > path = pdbg_target_path(tgt);
> > assert(path);
> > printf("%s", path);
> > free(path);
> > 
> > Solve this by adding a cache of the path to struct pdbg_target and
> > return
> > that instead.
> > 
> > Signed-off-by: Alistair Popple <alistair at popple.id.au>
> > ---
> > 
> >  libpdbg/device.c  |  7 +++++--
> >  libpdbg/libpdbg.h |  2 +-
> >  libpdbg/target.h  |  1 +
> >  src/path.c        |  9 ++-------
> >  src/ring.c        |  7 +------
> >  src/scom.c        | 11 ++---------
> >  6 files changed, 12 insertions(+), 25 deletions(-)
> > 
> > diff --git a/libpdbg/device.c b/libpdbg/device.c
> > index ad498dc..55d642e 100644
> > --- a/libpdbg/device.c
> > +++ b/libpdbg/device.c
> > @@ -774,9 +774,12 @@ bool pdbg_targets_init(void *fdt)
> > 
> >  	return true;
> >  
> >  }
> > 
> > -char *pdbg_target_path(struct pdbg_target *target)
> > +const char *pdbg_target_path(struct pdbg_target *target)
> > 
> >  {
> > 
> > -	return dt_get_path(target);
> > +	if (!target->path)
> > +		target->path = dt_get_path(target);
> > +
> > +	return target->path;
> > 
> >  }
> >  
> >  struct pdbg_target *pdbg_target_from_path(struct pdbg_target
> > 
> > *target, const char *path)
> > diff --git a/libpdbg/libpdbg.h b/libpdbg/libpdbg.h
> > index 5816150..69fa2c7 100644
> > --- a/libpdbg/libpdbg.h
> > +++ b/libpdbg/libpdbg.h
> > @@ -115,7 +115,7 @@ enum pdbg_target_status pdbg_target_status(struct
> > pdbg_target *target);
> > 
> >  void pdbg_target_status_set(struct pdbg_target *target, enum
> > 
> > pdbg_target_status status);
> > 
> >  bool pdbg_set_backend(enum pdbg_backend backend, const char
> > 
> > *backend_option);
> > 
> >  uint32_t pdbg_target_index(struct pdbg_target *target);
> > 
> > -char *pdbg_target_path(struct pdbg_target *target);
> > +const char *pdbg_target_path(struct pdbg_target *target);
> > 
> >  struct pdbg_target *pdbg_target_from_path(struct pdbg_target
> > 
> > *target, const char *path);
> > 
> >  uint32_t pdbg_parent_index(struct pdbg_target *target, char *klass);
> >  const char *pdbg_target_class_name(struct pdbg_target *target);
> > 
> > diff --git a/libpdbg/target.h b/libpdbg/target.h
> > index a8f777d..b8e1d99 100644
> > --- a/libpdbg/target.h
> > +++ b/libpdbg/target.h
> > @@ -36,6 +36,7 @@ struct pdbg_target {
> > 
> >  	char *name;
> >  	char *compatible;
> >  	char *class;
> > 
> > +	const char* path;
> > 
> >  	int (*probe)(struct pdbg_target *target);
> >  	void (*release)(struct pdbg_target *target);
> >  	uint64_t (*translate)(struct pdbg_target *target, uint64_t
> > 
> > addr);
> > diff --git a/src/path.c b/src/path.c
> > index a0eb666..b77ba79 100644
> > --- a/src/path.c
> > +++ b/src/path.c
> > @@ -339,14 +339,9 @@ bool path_target_all_selected(const char
> > *classname, struct pdbg_target *parent)
> > 
> >  void path_target_dump(void)
> >  {
> >  
> >  	struct pdbg_target *target;
> > 
> > -	char *path;
> > 
> > -	for_each_path_target(target) {
> > -		path = pdbg_target_path(target);
> > -		assert(path);
> > -		printf("%s\n", path);
> > -		free(path);
> > -	}
> > +	for_each_path_target(target)
> > +		printf("%s\n", pdbg_target_path(target));
> > 
> >  }
> >  
> >  struct pdbg_target *path_target_next(struct pdbg_target *prev)
> > 
> > diff --git a/src/ring.c b/src/ring.c
> > index 1e40db6..adafac2 100644
> > --- a/src/ring.c
> > +++ b/src/ring.c
> > @@ -39,17 +39,12 @@ static int get_ring(uint64_t ring_addr, uint64_t
> > ring_len)
> > 
> >  	assert(result);
> >  	
> >  	for_each_path_target_class("chiplet", target) {
> > 
> > -		char *path;
> > 
> >  		int rc, i, len;
> >  		
> >  		if (pdbg_target_status(target) != PDBG_TARGET_ENABLED)
> >  		
> >  			continue;
> > 
> > -		path = pdbg_target_path(target);
> > -		assert(path);
> > -
> > -		printf("%s: 0x%016" PRIx64 " = ", path, ring_addr);
> > -		free(path);
> > +		printf("%s: 0x%016" PRIx64 " = ",
> > pdbg_target_path(target), ring_addr);
> > 
> >  		rc = getring(target, ring_addr, ring_len, result);
> >  		if (rc) {
> > 
> > diff --git a/src/scom.c b/src/scom.c
> > index 6a59097..254c264 100644
> > --- a/src/scom.c
> > +++ b/src/scom.c
> > @@ -29,7 +29,7 @@
> > 
> >  int getscom(uint64_t addr)
> >  {
> >  
> >  	struct pdbg_target *target;
> > 
> > -	char *path;
> > +	const char *path;
> > 
> >  	uint64_t value;
> >  	int count = 0;
> > 
> > @@ -41,8 +41,6 @@ int getscom(uint64_t addr)
> > 
> >  			continue;
> >  		
> >  		path = pdbg_target_path(target);
> > 
> > -		assert(path);
> > -
> > 
> >  		xlate_addr = addr;
> >  		addr_base = pdbg_address_absolute(target, &xlate_addr);
> >  		if (!addr_base) {
> > 
> > @@ -52,12 +50,10 @@ int getscom(uint64_t addr)
> > 
> >  		if (pib_read(target, addr, &value)) {
> >  		
> >  			printf("p%d: 0x%016" PRIx64 " failed (%s)\n",
> > 
> > pdbg_target_index(addr_base), xlate_addr, path);
> > -			free(path);
> > 
> >  			continue;
> >  		
> >  		}
> >  		
> >  		printf("p%d: 0x%016" PRIx64 " = 0x%016" PRIx64 "
> > 
> > (%s)\n", pdbg_target_index(addr_base), xlate_addr, value, path);
> > -		free(path);
> > 
> >  		count++;
> >  	
> >  	}
> > 
> > @@ -68,7 +64,7 @@ OPTCMD_DEFINE_CMD_WITH_ARGS(getscom, getscom,
> > (ADDRESS));
> > 
> >  int putscom(uint64_t addr, uint64_t data, uint64_t mask)
> >  {
> >  
> >  	struct pdbg_target *target;
> > 
> > -	char *path;
> > +	const char *path;
> > 
> >  	int count = 0;
> >  	
> >  	for_each_path_target(target) {
> > 
> > @@ -80,8 +76,6 @@ int putscom(uint64_t addr, uint64_t data, uint64_t
> > mask)
> > 
> >  			continue;
> >  		
> >  		path = pdbg_target_path(target);
> > 
> > -		assert(path);
> > -
> > 
> >  		xlate_addr = addr;
> >  		addr_base = pdbg_address_absolute(target, &xlate_addr);
> >  		if (!addr_base) {
> > 
> > @@ -96,7 +90,6 @@ int putscom(uint64_t addr, uint64_t data, uint64_t
> > mask)
> > 
> >  		if (rc) {
> >  		
> >  			printf("p%d: 0x%016" PRIx64 " failed (%s)\n",
> > 
> > pdbg_target_index(addr_base), xlate_addr, path);
> > -			free(path);
> > 
> >  			continue;
> >  		
> >  		}
> 
> Amitay.