* [PATCH] zram: return error-valued pointer from zcomp_create()
@ 2014-03-05 19:56 Sergey Senozhatsky
2014-03-05 20:13 ` Andrew Morton
0 siblings, 1 reply; 3+ messages in thread
From: Sergey Senozhatsky @ 2014-03-05 19:56 UTC (permalink / raw)
To: Andrew Morton
Cc: Minchan Kim, Jerome Marchand, Nitin Gupta, linux-kernel,
Sergey Senozhatsky
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.
Reported-by: Jerome Marchand <jmarchan@redhat.com>
Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
---
drivers/block/zram/zcomp.c | 13 +++++++------
drivers/block/zram/zram_drv.c | 8 ++++----
2 files changed, 11 insertions(+), 10 deletions(-)
diff --git a/drivers/block/zram/zcomp.c b/drivers/block/zram/zcomp.c
index d591903..92a83df 100644
--- a/drivers/block/zram/zcomp.c
+++ b/drivers/block/zram/zcomp.c
@@ -319,9 +319,10 @@ void zcomp_destroy(struct zcomp *comp)
/*
* search available compressors for requested algorithm.
- * allocate new zcomp and initialize it. return NULL
- * if requested algorithm is not supported or in case
- * of init error
+ * allocate new zcomp and initialize it. return compressing
+ * backend pointer or ERR_PTR if things went bad. ERR_PTR(-EINVAL)
+ * if requested algorithm is not supported, ERR_PTR(-ENOMEM) in
+ * case of allocation error.
*/
struct zcomp *zcomp_create(const char *compress, int max_strm)
{
@@ -330,11 +331,11 @@ struct zcomp *zcomp_create(const char *compress, int max_strm)
backend = find_backend(compress);
if (!backend)
- return NULL;
+ return ERR_PTR(-EINVAL);
comp = kzalloc(sizeof(struct zcomp), GFP_KERNEL);
if (!comp)
- return NULL;
+ return ERR_PTR(-ENOMEM);
comp->backend = backend;
if (max_strm > 1)
@@ -343,7 +344,7 @@ struct zcomp *zcomp_create(const char *compress, int max_strm)
zcomp_strm_single_create(comp);
if (!comp->stream) {
kfree(comp);
- return NULL;
+ return ERR_PTR(-ENOMEM);
}
return comp;
}
diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
index e4d536b..9585957 100644
--- 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;
--
1.9.0.382.g7f3562c
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] zram: return error-valued pointer from zcomp_create()
2014-03-05 19:56 [PATCH] zram: return error-valued pointer from zcomp_create() Sergey Senozhatsky
@ 2014-03-05 20:13 ` Andrew Morton
2014-03-05 20:25 ` Sergey Senozhatsky
0 siblings, 1 reply; 3+ messages in thread
From: Andrew Morton @ 2014-03-05 20:13 UTC (permalink / raw)
To: Sergey Senozhatsky
Cc: Minchan Kim, Jerome Marchand, Nitin Gupta, linux-kernel
On Wed, 5 Mar 2014 22:56:55 +0300 Sergey Senozhatsky <sergey.senozhatsky@gmail.com> 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;
}
_
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] zram: return error-valued pointer from zcomp_create()
2014-03-05 20:13 ` Andrew Morton
@ 2014-03-05 20:25 ` Sergey Senozhatsky
0 siblings, 0 replies; 3+ messages in thread
From: Sergey Senozhatsky @ 2014-03-05 20:25 UTC (permalink / raw)
To: Andrew Morton
Cc: Sergey Senozhatsky, Minchan Kim, Jerome Marchand, Nitin Gupta,
linux-kernel
On (03/05/14 12:13), Andrew Morton wrote:
> On Wed, 5 Mar 2014 22:56:55 +0300 Sergey Senozhatsky <sergey.senozhatsky@gmail.com> 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;
> }
yes, looks fine to me. thank you.
Acked-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
-ss
> _
>
>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2014-03-05 20:29 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-03-05 19:56 [PATCH] zram: return error-valued pointer from zcomp_create() Sergey Senozhatsky
2014-03-05 20:13 ` Andrew Morton
2014-03-05 20:25 ` Sergey Senozhatsky
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox