Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753456AbYJALVc (ORCPT ); Wed, 1 Oct 2008 07:21:32 -0400 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S1752230AbYJALVY (ORCPT ); Wed, 1 Oct 2008 07:21:24 -0400 Received: from mtagate8.de.ibm.com ([195.212.29.157]:47111 "EHLO mtagate8.de.ibm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751770AbYJALVY (ORCPT ); Wed, 1 Oct 2008 07:21:24 -0400 Subject: Re: [patch 21/21] Add ioctl support for EMC Symmetrix Subsystem Control I/O From: Martin Schwidefsky Reply-To: schwidefsky@de.ibm.com To: Christoph Hellwig Cc: linux-kernel@vger.kernel.org, linux-s390@vger.kernel.org, Heiko Carstens , Nigel Hislop , Hannes Reinecke In-Reply-To: <20081001110301.GB6648@infradead.org> References: <20081001083317.782967473@de.ibm.com> <20081001084130.599948666@de.ibm.com> <20081001110301.GB6648@infradead.org> Content-Type: text/plain Organization: IBM Corporation Date: Wed, 01 Oct 2008 13:17:00 +0200 Message-Id: <1222859820.32562.8.camel@localhost> Mime-Version: 1.0 X-Mailer: Evolution 2.22.3.1 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2007 Lines: 70 On Wed, 2008-10-01 at 07:03 -0400, Christoph Hellwig wrote: > > +static int dasd_symm_io(struct dasd_device *device, void __user *argp) > > +{ > > + struct dasd_symmio_parms usrparm; > > + char *psf_data, *rssd_result; > > + struct dasd_ccw_req *cqr; > > + struct ccw1 *ccw; > > + int rc; > > + > > + /* Copy parms from caller */ > > + rc = copy_from_user(&usrparm, argp, sizeof(usrparm)) ? -EFAULT : 0; > > + if (rc) > > + goto out; > > That style is quite odd, what about the more normal > > rc = -EFAULT; > if (copy_from_user(&usrparm, argp, sizeof(usrparm))) > goto out; > > instead? Ok, I will change that. > > + /* alloc I/O data area */ > > + psf_data = kzalloc(usrparm.psf_data_len, GFP_KERNEL | GFP_DMA); > > + rssd_result = kzalloc(usrparm.rssd_result_len, GFP_KERNEL | GFP_DMA); > > + if (!psf_data || !rssd_result) { > > + rc = -ENOMEM; > > + goto out_free; > > + } > > just check each on individually and do individual unwinding. Makes the > code a little more easily readable. Well, matter of taste I would say. Two kzallocs, one check for the results and just two kfrees if one of the kzallocs failed. I find this version to be a little more compact and therefore easier to read. If you insist I will change it .. > > + /* get syscall header from user space */ > > + rc = copy_from_user(psf_data, > > + (void __user *)(unsigned long) usrparm.psf_data, > > + usrparm.psf_data_len) ? -EFAULT : 0; > > + if (rc) > > + goto out_free; > > Same as the first copy_from_user Ok. > > + rc = copy_to_user((void __user *)(unsigned long) usrparm.rssd_result, > > + rssd_result, usrparm.rssd_result_len) ? -EFAULT : 0; > > And here again. Ok. -- blue skies, Martin. "Reality continues to ruin my life." - Calvin. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/