| Message ID | 620b3961-6e05-6638-4ca8-18766acc91f4@users.sourceforge.net (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
> Subject: [PATCH] IB/qib: Use memdup_user() rather than duplicating its > diff --git a/drivers/infiniband/hw/qib/qib_fs.c I would be even more aggressive at reducing lines of code. For example do direct returns when ok to do: if (pos != 0 || count != sizeof(struct qib_flash)) return -EINVAL; tmp = memdup_user(buf, count); if (IS_ERR(tmp)) return PTR_ERR(tmp); The bail_tmp: label is then not needed. Mike
On Fri, Aug 19, 2016 at 09:06:20AM +0200, SF Markus Elfring wrote: > From: Markus Elfring <elfring@users.sourceforge.net> > Date: Fri, 19 Aug 2016 08:50:23 +0200 > > Reuse existing functionality from memdup_user() instead of keeping > duplicate source code. > > This issue was detected by using the Coccinelle software. > > Signed-off-by: Markus Elfring <elfring@users.sourceforge.net> Reviewed-by: Leon Romanovsky <leonro@mellanox.com>
On Fri, Aug 19, 2016 at 03:27:20PM +0000, Marciniszyn, Mike wrote: > > Subject: [PATCH] IB/qib: Use memdup_user() rather than duplicating its > > diff --git a/drivers/infiniband/hw/qib/qib_fs.c > > I would be even more aggressive at reducing lines of code. > > For example do direct returns when ok to do: > if (pos != 0 || count != sizeof(struct qib_flash)) > return -EINVAL; > > tmp = memdup_user(buf, count); > if (IS_ERR(tmp)) > return PTR_ERR(tmp); > > The bail_tmp: label is then not needed. You still need to free tmp allocation if qib_eeprom_write failed and this is your bail_tmp. 341 tmp = kmalloc(count, GFP_KERNEL); 342 if (!tmp) { 343 ret = -ENOMEM; 344 goto bail; 345 } 346 347 if (copy_from_user(tmp, buf, count)) { 348 ret = -EFAULT; 349 goto bail_tmp; 350 } 351 352 dd = private2dd(file); 353 if (qib_eeprom_write(dd, pos, tmp, count)) { 354 ret = -ENXIO; 355 qib_dev_err(dd, "failed to write to flash\n"); 356 goto bail_tmp; 357 } > > Mike >
> > > > The bail_tmp: label is then not needed. > > You still need to free tmp allocation if qib_eeprom_write failed and this is > your bail_tmp. > Typo. The bail: label is not needed. Mike -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Aug 19, 2016 at 03:42:29PM +0000, Marciniszyn, Mike wrote: > > > > > > The bail_tmp: label is then not needed. > > > > You still need to free tmp allocation if qib_eeprom_write failed and this is > > your bail_tmp. > > > > Typo. The bail: label is not needed. Yeah, it makes sense. Thanks > > Mike
On 8/19/2016 11:27 AM, Marciniszyn, Mike wrote: >> Subject: [PATCH] IB/qib: Use memdup_user() rather than duplicating its >> diff --git a/drivers/infiniband/hw/qib/qib_fs.c > > I would be even more aggressive at reducing lines of code. > > For example do direct returns when ok to do: > if (pos != 0 || count != sizeof(struct qib_flash)) > return -EINVAL; > > tmp = memdup_user(buf, count); > if (IS_ERR(tmp)) > return PTR_ERR(tmp); > > The bail_tmp: label is then not needed. > > Mike > With Mike's additional cleanups in place, patch applied.
diff --git a/drivers/infiniband/hw/qib/qib_fs.c b/drivers/infiniband/hw/qib/qib_fs.c index fcdf3791..910f0d9 100644 --- a/drivers/infiniband/hw/qib/qib_fs.c +++ b/drivers/infiniband/hw/qib/qib_fs.c @@ -338,17 +338,12 @@ static ssize_t flash_write(struct file *file, const char __user *buf, goto bail; } - tmp = kmalloc(count, GFP_KERNEL); - if (!tmp) { - ret = -ENOMEM; + tmp = memdup_user(buf, count); + if (IS_ERR(tmp)) { + ret = PTR_ERR(tmp); goto bail; } - if (copy_from_user(tmp, buf, count)) { - ret = -EFAULT; - goto bail_tmp; - } - dd = private2dd(file); if (qib_eeprom_write(dd, pos, tmp, count)) { ret = -ENXIO;