Re: [PATCH v2 3/5] IB/hfi1: Add ioctl() interface for user commands

[Doug Ledford]

On 05/12/2016 03:40 PM, Jason Gunthorpe wrote:
> On Thu, May 12, 2016 at 03:27:27PM -0400, Dennis Dalessandro wrote:
>>> I thought we agreed to get rid of this as well? It certainly does not
>>> belong here, and as a general rule, I don't think ioctls should be
>>> doing capable tests..
>>
>> Yeah. I left it in this patch set because this just "ports" our existing
>> code to ioctl(). The eprom stuff is completely removed in another patch set
>> that I posted shortly after this. It's at:
> 
> Adding code and then removing it in a later patch is not a best
> practice.. Just don't add it or define ioctl numbers at all..

Yeah, but then that changes the nature of the patchset.  It goes from
being "We ported the existing write API to ioctl API" to "We ported
existing write API to ioctl API and also changed the scope of the API in
the process", which is considered bad coding practice (one stand alone
change per commit).  It's pretty 6 of one, half dozen of the other if
you ask me.  A better solution would have been to remove the EEPROM
stuff from the write ioctl, then only port the remaining stuff.  That
would have avoided both coding issues, but I also understand how they
got here, which is they did the ioctl conversion before they knew they
were going to rip out the EEPROM code.  In any case, the best fix would
be to rebase the two series that are remaining and move any "rip out
things like eeprom support" patches to prior to the ioctl patches and
make it so that they rip out the write interface version of it instead,
and then squash a second copy of the ioctl removal into this patch.

-- 
Doug Ledford <dledford@xxxxxxxxxx>
              GPG KeyID: 0E572FDD

Attachment: signature.asc
Description: OpenPGP digital signature