| Message ID | 20200110043413.31036-1-alistair@popple.id.au |
|---|---|
| State | Accepted |
| Headers | show |
| Series | libpdbg: Make pdbg_target_path() return a const string | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | warning | Failed to apply on branch master (8b4611b5d8e7e2279fe4aa80c892fcfe10aa398d) |
| snowpatch_ozlabs/apply_patch | fail | Failed to apply to any branch |
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@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.
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@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.
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; }
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@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(-)