sowmini,
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?
bge_chip2.c:
1937: is this meant to be 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.
mac.c:
52: is this header not needed?
66: unless this is used in multiple places. I suggest you put this
string back into the cmn_err() at 929.
72-73: seems like these are not used.
923: delete blank line.
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.
linkprop.c:
47: why include dls.h?
81-96:
these declarations could be grouped.
e.g. static pd_checkf_t f1,f2,f3;
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)
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)
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.
