On 03/22/2016 08:45 PM, Dan Carpenter wrote:
Hello Wenwei Tao,
The patch da1e284919b0: "lightnvm: add a bitmap of luns" from Mar 3,
2016, has bad error handling.
Error handling should be simple and mindless. Meanwhile we have a
kcalloc() in one function and kfree() int the grandparent caller
function. Plus it uses One Err styles error handling with just a single
label so you know it's going to be wrong from the get go.
Each allocation function should have a mirror free function:
nvm_init() -> nvm_uninit()
nvm_core_init() -> nvm_core_uninit()
drivers/lightnvm/core.c
549 dev = kzalloc(sizeof(struct nvm_dev), GFP_KERNEL);
550 if (!dev)
551 return -ENOMEM;
552
553 dev->q = q;
554 dev->ops = ops;
555 strncpy(dev->name, disk_name, DISK_NAME_LEN);
556
557 ret = nvm_init(dev);
558 if (ret)
559 goto err_init;
Every function should handle its own errors. We should not free things
which have not been allocated. This means we will need multiple error
labels with clear names.
goto free_dev;
560
561 if (dev->ops->max_phys_sect > 256) {
562 pr_info("nvm: max sectors supported is 256.\n");
563 ret = -EINVAL;
564 goto err_init;
This should be:
goto uninit_dev;
And obviously you can tell from the name that it will undo the
nvm_init().
Thanks Dan for reviewing. I appreciate you taking a look.
565 }
566
567 if (dev->ops->max_phys_sect > 1) {
568 dev->ppalist_pool = dev->ops->create_dma_pool(dev, "ppalist");
569 if (!dev->ppalist_pool) {
570 pr_err("nvm: could not create ppa pool\n");
571 ret = -ENOMEM;
572 goto err_init;
This is still "goto uninit_dev;" because we haven't allocated anything
else, since nvm_init().
573 }
574 }
575
576 if (dev->identity.cap & NVM_ID_DCAP_BBLKMGMT) {
577 ret = nvm_get_sysblock(dev, &dev->sb);
578 if (!ret)
579 pr_err("nvm: device not initialized.\n");
580 else if (ret < 0)
581 pr_err("nvm: err (%d) on device initialization\n", ret);
There should be error handling here, instead of printing an error and
continuing. The last thing we allocated is the dma_pool so it's
"goto delete_dma_pool;". The name is clear.
From
576 if (dev->identity.cap & NVM_ID_DCAP_BBLKMGMT) {
and onwards have successfully initialized the device. The get_sysblock
is okay to fail, and the media manager (nvm_init_mgr) may return NULL,
as it can initialized later. When a new media manager module is loaded,
devices without a media manager is probed for support.
This properly warrants at least a comment. Would that be enough or would
you advice another method for discovery of media managers attached to a
device.
It currently probes at:
nvm_register() (On device registration)
__nvm_ioctl_dev_init() (When dev is initialized with a media manager)
nvm_register_mgr (When a new media manager is registered)
This could be done asynchronously, but I have preferred it to be
synchronous.
582 }
583
584 /* register device with a supported media manager */
585 down_write(&nvm_lock);
586 if (ret > 0)
587 dev->mt = nvm_init_mgr(dev);
We could check for NULL here. "goto_put_sysblock;"
588 list_add(&dev->devices, &nvm_devices);
589 up_write(&nvm_lock);
590
591 return 0;
592 err_init:
593 kfree(dev->lun_map);
594 kfree(dev);
595 return ret;
The unwind code should be a mirror of the wind up code. We use the same
if statements.
Thanks. I didn't catch this one.
put_sysblock:
if (dev->identity.cap & NVM_ID_DCAP_BBLKMGMT)
nvm_put_sysblock(dev, &dev->sb);
delete_dma_pool:
if (dev->ops->max_phys_sect > 1)
dev->ops->delete_dma_pool();
uninit_dev:
nvm_uninit(dev);
free_def:
kfree(dev);
return ret;
This way is more clear. In the original code we don't free dev->lptbl
and other stuff and the code is confusing and full of layer violations
and incomplete.
596 }
regards,
dan carpenter
--
To unsubscribe from this list: send the line "unsubscribe linux-block" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
