On (03/05/14 12:13), Andrew Morton wrote: > 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; > }
yes, looks fine to me. thank you. Acked-by: Sergey Senozhatsky <[email protected]> -ss > _ > > -- 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/
