James Carlson wrote:
> Garrett D'Amore writes:
>
>> There is a closed version, which includes one closed file fix, at
>>
>> http://jurassic.sfbay/~gd78059/webrev/mblkl
>>
>
> Thanks for doing this!
>
> uts/common/fs/smbclnt/netsmb/smb_trantcp.c
>
> I don't think I'd add a copyright message here, but check with the
> gatekeepers. The usual rule is that if it's just changes in
> commentary or "trivial" matters like lint warnings, then you don't
> add or change the copyright notice.
>
Hmm... I don't know. My understanding was that any change to the source
code, apart from licensing, required an updated copyright. I don't
really care one way or the other. :-)
> uts/common/inet/vni/vni.c
> uts/common/io/aggr/aggr_ctl.c
> uts/common/io/ath/ath_aux.c
>
> I don't see where these (and several others I didn't list) have any
> MBLKL changes in them, or where the bug you're citing includes these
> changes. I agree that the clean-up is nice, though. Perhaps you'll
> want to add a note to your Evaluation.
>
Yeah, I need to have more in the bug, or create new bugs. Creating
separate CRs for all this random cleanup is a PITA, so I'll try to
leverage the existing CR. The CRT may ask me to file separate ones... I
hope not.
> uts/common/io/dmfe/dmfe_mii.c
>
> 258: this seems a little random.
>
It fixes an incorrect return at line 280, where the previously 32-bit
"data" is returned through a uint16_t. This was causing a lint
complaint when I removed one of the LINTTAGS. MII registers are by
definition only 16-bits wide. :-)
> uts/common/io/ib/mgt/ibcm/ibcm_arp_link.c
>
> 573: why is the /* LINTED */ no longer needed? I don't see a void *
> cast on this one.
>
Good point. I need to fix that. Interestingly enough, I didn't update
the Makefile for ibcm, so the lint warning that I should have seen was
suppressed. (So the question is, why have these /*LINTED*/ in the
source files, *and* a LINTTAGS that disables the corresponding warning!
Hah!) I've not claimed to fix all the possible warnings in the module,
so I'm not going to edit the Makefile. The other ones I got just by
searching for /*LINTED*/.
> uts/common/io/rge/rge_chip.c
>
> 299: what's this?
>
Ah, I assumed that the function was put in place to help with debugging
from kmdb (otherwise I could have removed it). Making it not static
shuts up lint. (It allows that the function may be called in code
external to this, which is precisely what is needed if this is to be
usable from kmdb.) I *was* tempted to just remove the function
altogether, but I felt that the rge maintainers and/or RPE might object
to such.
> uts/common/io/usb/hcd/uhci/uhcipolled.c
>
> 258: not sure what changed here. (?)
>
Cstyle formatting. You can't see the indent changes that resulted.
(webrev suppresses indent changes from the review. I wound up joining
the lines because they fit cleaner, though I could have skipped that.)
This was a continuation line indent fix.
> uts/common/io/vuidmice/vuidmice.c
>
> 676: this has an extra (caddr_t) cast that can be dropped. (The
> result of dereferencing (caddr_t *) is just caddr_t.)
>
Agreed. There were several other places where pointless casts were used
that I removed. Sometimes I think people cast just out of
habit/laziness, rather than for any real need.
> 724: this shouldn't have passed cstyle; missing space between "void"
> and "*".
>
Agreed, but it did! I will fix (this file, not cstyle!)
> uts/common/io/vuidmice/vuidps2.c
>
> 519: I think the better way to write this is mp->b_wptr >
> mp->b_rptr. Comparing MBLKL against zero seems odd to me.
>
Agreed. I think I used a similar approach in one of the other changes.
> uts/common/io/wscons.c
>
> 710: random?
>
Yeah. While I was visually inspecting cstyle fixes in that file, I
noticed that it would fit well without the extra line wrap, so I removed
it.
> uts/common/ktli/t_kconnect.c
>
> 196: is the cast still needed?
>
*Probably* not. The difference between two uintptr_t's might not fit
within an int (depends on platform), so this makes it explicit, and
possibly suppresses a 64-bit lint warning. I didn't think it was worth
the brain power to figure out whether it was required or not. (So now
*I'm* the one being lazy. :-)
> uts/common/ktli/t_krcvudat.c
>
> 220: cast needed?
>
Ditto.
> uts/intel/amd8111s/Makefile
>
> 56-59: I'd remove the empty section as well.
>
One could argue that the LDFLAGS += is an override. I wasn't sure, so I
left it alone.
> uts/intel/pcwl/Makefile
>
> 75: remove
>
Huh? You better clarify that one for me.
> uts/intel/wpi/Makefile
>
> 61: why do *some* of the Makefiles still have E_BAD_PTR_CAST_ALIGN?
> It looks like you went to some trouble to rip these out. Are
> some lint nits still left, or are the Makefiles incorrectly
> suppressing these warnings?
>
I went to some trouble for some of the drivers, but not all of them.
The ones that I didn't take it out of had too many warnings, and I
decided ENOTENOUGHTIME to get it done.
Put another way, I wanted to do a few of the drivers (especially the few
that I am responsible for, such as dmfe) to set an example, but I didn't
want to get bogged down with a commitment to make all of uts, or even
necessarily all of a given driver clean. This is just a step in the
right direction, but I don't claim it is a complete solution.
-- Garrett
_______________________________________________
driver-discuss mailing list
driver-discuss@opensolaris.org
http://mail.opensolaris.org/mailman/listinfo/driver-discuss
