Re: [Patch]mac80211: Add support for mesh proxy path dump

[Johannes Berg <johannes@sipsolutions.net>]

On Wed, 2014-09-03 at 14:12 +0200, Henning Rogge wrote:
> On Wed, Sep 3, 2014 at 1:52 PM, Johannes Berg <johannes@sipsolutions.net> wrote:
> > On Mon, 2014-09-01 at 13:26 +0200, Henning Rogge wrote:
> >> The following patch adds NL80211_CMD_GET_MPP as a new nl80211 command that
> >> allows to query the content of the 'mesh proxy path' table of mac80211s via
> >> 'get' or 'dump'  operation.
> >
> > For review and merging, and to make it more obvious to you as well when
> > writing commit logs/documentation/etc I'd prefer if you'd split this up
> > into separate cfg80211 and mac80211 patches.
> 
> Just to make sure I got the split right...
> 
> First patch to define the two new cfg80211_ops and the necessary
> nl80211/tracing helper functions, second patch that implements
> get_mpp() and dump_mpp() functions?

Right. That way you have one patch that defines the API etc., and
another that implements it, and it's easier to review and document the
APIs in the right places etc.

I don't always insist on this, but it's a pretty big patch so I'd prefer
it this way (and it also should be almost trivial to split)

> >> +     int     (*get_mpp)(struct wiphy *wiphy, struct net_device *dev,
> >> +                          u8 *dst, u8 *mpp, struct mpath_info *pinfo);
> >> +     int     (*dump_mpp)(struct wiphy *wiphy, struct net_device *dev,
> >> +                           int idx, u8 *dst, u8 *mpp,
> >> +                           struct mpath_info *pinfo);
> >
> > Should dst/mpp be const? Or are those output parameters?
> 
> Yes, similar to the get_mpath/dump_mpath cfg80211_ops these are output
> parameters. Same thing for pinfo.

Yeah I figured pinfo was, wasn't sure about the others :)

> > Should it really be mpath_info? I thought this was some other thing?
> > Probably just need more documentation :)
> 
> mpath and mpp use the same storage object (struct mesh_path), so I
> reused the mpath_info struct. As far as I can see its just a "transfer
> object" between for cfg80211 that temporarily stores data from the
> cfg80211_ops for nl80211 messages.

Indeed, it is.

> Shall I create a new struct for the two new cfg80211_ops?

No need, I just wasn't sure it really needed the same data so figured
I'd ask.

> See

I guess you got cut off there :)

johannes