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

[Amitay Isaacs]

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;
>  		}
>  
> -- 
> 2.20.1
> 

Amitay.
-- 

Of all the things I have lost, I miss my mind the most.