On Wed, 5 Mar 2014 22:56:55 +0300 Sergey Senozhatsky
<[email protected]> wrote:
> Instead of returning just NULL, return ERR_PTR from zcomp_create()
> if compressing backend creation has failed. ERR_PTR(-EINVAL) for
> unsupported compression algorithm request, ERR_PTR(-ENOMEM) for
> allocation (zcomp or compression stream) error.
>
> Perform IS_ERR() check of returned from zcomp_create() value in
> disksize_store() and set return code to PTR_ERR().
>
> Change suggested by Jerome Marchand.
>
> ...
>
> --- a/drivers/block/zram/zram_drv.c
> +++ b/drivers/block/zram/zram_drv.c
> @@ -583,7 +583,7 @@ static ssize_t disksize_store(struct device *dev,
> struct zcomp *comp;
> struct zram_meta *meta;
> struct zram *zram = dev_to_zram(dev);
> - int err = -EINVAL;
> + int err = -EBUSY;
>
> disksize = memparse(buf, NULL);
> if (!disksize)
> @@ -595,9 +595,10 @@ static ssize_t disksize_store(struct device *dev,
> return -ENOMEM;
>
> comp = zcomp_create(zram->compressor, zram->max_comp_streams);
> - if (!comp) {
> + if (IS_ERR(comp)) {
> pr_info("Cannot initialise %s compressing backend\n",
> zram->compressor);
> + err = PTR_ERR(comp);
> goto out_cleanup;
> }
>
> @@ -605,7 +606,6 @@ static ssize_t disksize_store(struct device *dev,
> if (init_done(zram)) {
> up_write(&zram->init_lock);
> pr_info("Cannot change disksize for initialized device\n");
> - err = -EBUSY;
> goto out_cleanup;
> }
>
> @@ -618,7 +618,7 @@ static ssize_t disksize_store(struct device *dev,
> return len;
>
> out_cleanup:
> - if (comp)
> + if (!IS_ERR(comp))
> zcomp_destroy(comp);
> zram_meta_free(meta);
> return err;
The disksize_store() error ends up being unnecessarily inconsistent in
its handling of `err' and in its freeing/unlocking flow. How about
doing it this way?
static ssize_t disksize_store(struct device *dev,
struct device_attribute *attr, const char *buf, size_t len)
{
u64 disksize;
struct zcomp *comp;
struct zram_meta *meta;
struct zram *zram = dev_to_zram(dev);
int err;
disksize = memparse(buf, NULL);
if (!disksize)
return -EINVAL;
disksize = PAGE_ALIGN(disksize);
meta = zram_meta_alloc(disksize);
if (!meta)
return -ENOMEM;
comp = zcomp_create(zram->compressor, zram->max_comp_streams);
if (IS_ERR(comp)) {
pr_info("Cannot initialise %s compressing backend\n",
zram->compressor);
err = PTR_ERR(comp);
goto out_free_meta;
}
down_write(&zram->init_lock);
if (init_done(zram)) {
pr_info("Cannot change disksize for initialized device\n");
err = -EBUSY;
goto out_destroy_comp;
}
zram->meta = meta;
zram->comp = comp;
zram->disksize = disksize;
set_capacity(zram->disk, zram->disksize >> SECTOR_SHIFT);
up_write(&zram->init_lock);
return len;
out_destroy_comp:
up_write(&zram->init_lock);
zcomp_destroy(comp);
out_free_meta:
zram_meta_free(meta);
return err;
}
---
a/drivers/block/zram/zram_drv.c~zram-return-error-valued-pointer-from-zcomp_create-fix
+++ a/drivers/block/zram/zram_drv.c
@@ -583,7 +583,7 @@ static ssize_t disksize_store(struct dev
struct zcomp *comp;
struct zram_meta *meta;
struct zram *zram = dev_to_zram(dev);
- int err = -EBUSY;
+ int err;
disksize = memparse(buf, NULL);
if (!disksize)
@@ -599,14 +599,14 @@ static ssize_t disksize_store(struct dev
pr_info("Cannot initialise %s compressing backend\n",
zram->compressor);
err = PTR_ERR(comp);
- goto out_cleanup;
+ goto out_free_meta;
}
down_write(&zram->init_lock);
if (init_done(zram)) {
- up_write(&zram->init_lock);
pr_info("Cannot change disksize for initialized device\n");
- goto out_cleanup;
+ err = -EBUSY;
+ goto out_destroy_comp;
}
zram->meta = meta;
@@ -617,9 +617,10 @@ static ssize_t disksize_store(struct dev
return len;
-out_cleanup:
- if (!IS_ERR(comp))
- zcomp_destroy(comp);
+out_destroy_comp:
+ up_write(&zram->init_lock);
+ zcomp_destroy(comp);
+out_free_meta:
zram_meta_free(meta);
return err;
}
_
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
