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
