----- Forwarded message from Sowmini.Varadhan at Sun.COM -----
> To: Eric Cheng <tlc at sun.com>
> Cc: Sowmini.Varadhan at Sun.COM
> From: Sowmini.Varadhan at Sun.COM
> Date: Sun, 02 Dec 2007 16:13:27 -0500
> Subject: Re: [networking-discuss] Brussels webrev updated with review comments
> In-reply-to: <20071201233820.GA14068 at izuko.sfbay.sun.com>
> X-Authentication-warning: quasimodo.East.Sun.COM: sowmini set sender to
> sowmini.varadhan at sun.com using -f
> User-Agent: Mutt/1.5.16 (2007-06-11)
> Original-recipient: rfc822;sowmini at amer5-mail1.Central.Sun.COM
>
>
> Eric,
>
> thanks very much for taking the time to review this.
>
> Here are my responses:
>
> >
> > here are my comments based on your latest webrev:
> >
> > dladm.c:
> > in a few places here, you added bitfields to structures.
> > in some of these structures, boolean_t is used for other things.
> > can you change the new bitfields to boolean just to keep things
> > consistent?
>
> Accept.
>
> > bge_chip2.c:
> > 1937: is this meant to be removed?
>
> yes, removed.
>
> >
> > dld_drv.c:
> > 429-473:
> > these two functions can be refactored.
> > the dls code can be moved here.
> > just add a new function called:
> > drv_ioc_prop_common(dld_ctl_str_t *ctls, mblk_t *mp, boolean_t set)
> > {
> > .....
> > if (set)
> > mac_set_prop(...);
> > else
> > mac_get_prop(...);
> > .....
> > }
> > everything other than what's shown above are common code.
>
> Ok. Seb had mentioned that there are some layering
> conventions where dld does not directly invoke mac functions,
> but I see that drv_ioc_attr does this, and this results in
> some simplification of the code path, so I see no issues with
> your suggestion.
>
> > mac.c:
> > 52: is this header not needed?
>
> not needed. will be removed.
>
> > 66: unless this is used in multiple places. I suggest you put this
> > string back into the cmn_err() at 929.
>
> Accepted.
>
> > 72-73: seems like these are not used.
> Accepted.
>
> > 923: delete blank line.
> Accepted.
>
> > 2490,2504:
> > I find it odd that a mac layer interface is taking something called
> > dld_* as an argument. also, only a few fields in the structure are
> > used. it might be simpler to just expand the arglist and change
> > the signature to:
> >
> > mac_set_prop(mac_handle_t mh, const char *name, mac_public_prop_t prop,
> > void *val, uint_t valsize);
> >
> > dld_prnum_t should be changed to mac_public_prop_t and moved to mac.h.
> >
> > once the above is done, bge should not need to include dld.h.
>
> While this is true for public properties, both the pr_name and
> the pr_number are needed (private properties need the pr_name)
> So the signature will be changed as you suggest, except that
> mac_prop_t will be defined as
>
> typedef struct mac_prop_s {
> dld_prnum_t mac_pr_num
> char *mac_pr_name;
> } mac_prop_t;
>
> that will be passed from dld_ioc_prop_common to mac_{set,get}_prop
>
> >
> >
> > linkprop.c:
> > 47: why include dls.h?
>
> accept. no need to include this.
>
> >
> > 81-96:
> > these declarations could be grouped.
> > e.g. static pd_checkf_t f1,f2,f3;
>
> accept.
>
> > 116-133:
> > can we not use these #defines?
> > I think the strings could be used directly in the declarations.
> > I understand these are used in multiple places. but these strings
> > are not going to change drastically so there is little reason to
> > have #defines for them. (also, if these strings were to change,
> > the #defines themselves would likely have to be renamed as well,
> > which nullifies any benefits of using #defines)
>
> accept.
>
> > general issues with linkprop.c:
> > 1. it seems that all the pd_get entry points are implemented wrong.
> > pd_get is supposed to fill in the prop_val buffer provided
> > by the caller, not overwrite the prop_val pointer with something
> > else. you leak memory with every dladm_get_prop() call because
> > the buffer you allocated in the pd_get routines do not get freed.
> > (dladm_get_prop() was meant to use the caller-allocate model so
> > don't add any "free" calls in dladm)
>
> Ok. A related note here is that it seems that the prop_val buffer
> passed to pd_get should be at least DLADM_PROP_VAL_MAX; I'll
> make sure this is explicitly commented in libdllink.h
>
> > 2. the usage of val_desc_t is not consistent with how libdlwlan.c
> > uses it. val_desc_t has a vd_val and it's meant to hold the numeric
> > value corresponding to value name vd_name. in your code, this is
> > not used. instead, you fill vd_val with the ioctl buffer. I think
> > the buffer allocation could be deferred to pd_set.
> >
> > here is breakdown of what pd_check/set are supposed to do:
> >
> > pd_check() checks that the input values are valid. it does so by
> > iterating through the pd_modval list of this property. if the
> > modifiable values cannot be expressed as a list, a pd_check specific
> > to this property can be used. if the input values are verified to be
> > valid, pd_check allocates a val_desc_t fills it with either a
> > val_desc_t found on the pd_modval list or something generated on
> > the fly.
> >
> > pd_set() takes val_desc_t given by pd_check(), translates it into a
> > format suitable for kernel consumption. this may require allocation
> > of buffers..etc. pd_set() may call another common routine (used by
> > all other pd_sets) which invokes the ioctl with the filled ioctl
> > buffer.
>
> Ok. I've also added above notes to linkprop.c for future reference.
> A related point to note here is that UV's autopush implementation
> also violates the above somewhat, when it allocates the ioctl buffer
> itself in the pd_check.
>
> --Sowmini
----- End forwarded message -----
