[Pdbg] [PATCH] libpdbg: Add an api to get target address by class

[Amitay Isaacs]

On Wed, 2018-11-07 at 15:51 +1100, Alistair Popple wrote:
> On Monday, 5 November 2018 12:15:25 PM AEDT Amitay Isaacs wrote:
> > On Tue, 2018-10-23 at 14:35 +1100, Alistair Popple wrote:
> > > Signed-off-by: Alistair Popple <alistair at popple.id.au>
> > > ---
> > > 
> > >  libpdbg/libpdbg.h |  1 +
> > >  libpdbg/target.c  | 10 ++++++++++
> > >  2 files changed, 11 insertions(+)
> > > 
> > > diff --git a/libpdbg/libpdbg.h b/libpdbg/libpdbg.h
> > > index b1d6655..ce11fff 100644
> > > --- a/libpdbg/libpdbg.h
> > > +++ b/libpdbg/libpdbg.h
> > > @@ -91,6 +91,7 @@ const char *pdbg_target_dn_name(struct
> > > pdbg_target
> > > *target);
> > > 
> > >  void *pdbg_target_priv(struct pdbg_target *target);
> > >  void pdbg_target_priv_set(struct pdbg_target *target, void
> > > *priv);
> > >  struct pdbg_target *pdbg_target_root(void);
> > > 
> > > +struct pdbg_target *pdbg_class_target_addr(struct pdbg_target
> > > *target, const char *name, uint64_t *addr);
> > > 
> > >  /* Procedures */
> > >  int fsi_read(struct pdbg_target *target, uint32_t addr, uint32_t
> > > 
> > > *val);
> > > diff --git a/libpdbg/target.c b/libpdbg/target.c
> > > index 975ecec..fbcf792 100644
> > > --- a/libpdbg/target.c
> > > +++ b/libpdbg/target.c
> > > @@ -32,6 +32,16 @@ static struct pdbg_target
> > > *get_class_target_addr(struct pdbg_target *target, con
> > > 
> > >  	return target;
> > >  
> > >  }
> > > 
> > > +struct pdbg_target *pdbg_class_target_addr(struct pdbg_target
> > > *target, const char *name, uint64_t *addr)
> > > +{
> > > +	uint64_t tmp;
> > > +
> > > +	if (!addr)
> > > +		addr = &tmp;
> > > +
> > > +	return get_class_target_addr(target, name, addr);
> > > +}
> > > +
> > 
> > I think this api is badly named.  Isn't this same as
> > 
> >   parent = pdbg_target_parent(target, name)
> >   pdbg_get_address(parent, &addr)
> > 
> > May be we don't want to add an api that does the same thing as the
> > combination of existing apis.  Unless there is a specific corner
> > case
> > which is not covered.
> 
> I think you're right. The less API functions to maintain the better,
> it's not 
> an especially common usecase so I will drop this.

I think I read the code wrong.  It's doing address translation relative
to a particular parent of specified class.  It's not the same as
pdbg_get_address(parent, &addr).

Since it's doing address translation, I don't think there should be any
users for such a functionality outside libpdbg.  If there is a need for
such a functionality, it probably means we have some missing abstration
in libpdbg.

Amitay.
-- 

Challenges can be stepping stones or stumbling blocks. It's just a matter of
how you view them.