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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Thu, 12 May 2016 15:48:16 -0400
From:	Doug Ledford <dledford@...hat.com>
To:	Jason Gunthorpe <jgunthorpe@...idianresearch.com>,
	Dennis Dalessandro <dennis.dalessandro@...el.com>
Cc:	Mike Marciniszyn <mike.marciniszyn@...el.com>,
	linux-rdma@...r.kernel.org,
	Mitko Haralanov <mitko.haralanov@...el.com>,
	linux-kernel@...r.kernel.org, linux-fsdevel@...r.kernel.org,
	Ira Weiny <ira.weiny@...el.com>
Subject: Re: [PATCH v2 3/5] IB/hfi1: Add ioctl() interface for user commands

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@...hat.com>
              GPG KeyID: 0E572FDD

Download attachment "signature.asc" of type "application/pgp-signature" (885 bytes)