[brussels-dev] Fwd: Re: Brussels- nddcompat code review

[sowmini.varadhan at sun.com]

Artem,

thanks much for the review.

> linkprop.c:
  :
> 1949                         return (dladm_errno2status(errno));
>
> fd leak.

accepted fixed.

   :
> If i_dladm_ioctl() returns value from ioctl(), it probably shouldn't be  
> assigned to dladm_status_t status?

accepted fixed.

> mac_ndd.c:
>
> 196                 mp->b_cont = allocb(len, BPRI_HI); /* XXX allocate >  
> len? */
>
> XXX means something's wrong here?

was me being indecisive. the IP version of this code allocates in large chunks, and 
I was wondering if I should retain that behavior. I've made the choice
of allocating only as much as I need.

>  309         mp1 = allocb(avail, BPRI_HI); /* the returned buffer */
>  ...
>  403         status = mip->mi_callbacks->mc_getprop(mip->mi_driver,  
> priv_name,
>  404             DLD_PROP_PRIVATE, 0, avail, mp1->b_rptr);
>  ...
>  411         size_out += strnlen((const char *)mp1->b_rptr, avail);
>  412         valp += size_out;
>  413         *valp++ = '\0'; /* need \0\0 */
>  414         *valp++ = '\0';
>
> If driver returns a string of length avail-1, wouldn't line 414 overflow  
> the buffer?

good catch. I will pass in avail-2, and also checking that avail is at least
2 in this function.

> I'm not sure the project codename will be remembered for long and make  
> sense to most people. I think if we take out "Brussels driver", the rest  
> of the comment won't lose much, it's pretty obvious code.

Fixed.

>
> 1884         mpriv = mip->mi_priv_prop;
> 1885         kmem_free(mpriv, mip->mi_npriv_prop * sizeof (*mpriv));
>
> It took me a few seconds to register that "n". Variable names should  
> differ in more than one character, imho. I'm not feeling strongly about  
> this - up to you.

You have a good  point. There is some precedent in mi_kstat_count and 
mt_statcount, so I shall call this mi_priv_prop_count to make  the
distinction clear.

--Sowmini