linux-kernel - Re: [PATCH 09/11] btrfs: change set_level() to bound the level passed in

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20190130220656.GA50584@dennisz-mbp.dhcp.thefacebook.com>
Date:   Wed, 30 Jan 2019 17:06:56 -0500
From:   Dennis Zhou <dennis@...nel.org>
To:     Nikolay Borisov <nborisov@...e.com>
Cc:     Dennis Zhou <dennis@...nel.org>, David Sterba <dsterba@...e.com>,
        Josef Bacik <josef@...icpanda.com>, Chris Mason <clm@...com>,
        Omar Sandoval <osandov@...ndov.com>,
        Nick Terrell <terrelln@...com>, kernel-team@...com,
        linux-btrfs@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 09/11] btrfs: change set_level() to bound the level
 passed in

On Tue, Jan 29, 2019 at 10:14:18AM +0200, Nikolay Borisov wrote:
> 
> 
> On 28.01.19 г. 23:24 ч., Dennis Zhou wrote:
> > Currently, the only user of set_level() is zlib which sets an internal
> > workspace parameter. As level is now plumbed into get_workspace(), this
> > can be handled there rather than separately.
> > 
> > This repurposes set_level() to bound the level passed in so it can be
> > used when setting the mounts compression level and as well as verifying
> > the level before getting a workspace. The other benefit is this divides
> > the meaning of compress(0) and get_workspace(0). The former means we
> > want to use the default compression level of the compression type. The
> > latter means we can use any workspace available.
> > 
> > Signed-off-by: Dennis Zhou <dennis@...nel.org>
> > ---
 >  }
> > diff --git a/fs/btrfs/compression.h b/fs/btrfs/compression.h
> > index e3627139bc5c..d607be40aa0e 100644
> > --- a/fs/btrfs/compression.h
> > +++ b/fs/btrfs/compression.h
> > @@ -90,7 +90,7 @@ blk_status_t btrfs_submit_compressed_write(struct inode *inode, u64 start,
> >  blk_status_t btrfs_submit_compressed_read(struct inode *inode, struct bio *bio,
> >  				 int mirror_num, unsigned long bio_flags);
> >  
> > -unsigned btrfs_compress_str2level(const char *str);
> > +unsigned int btrfs_compress_str2level(unsigned int type, const char *str);
> >  
> >  enum btrfs_compression_type {
> >  	BTRFS_COMPRESS_NONE  = 0,
> > @@ -149,7 +149,7 @@ struct btrfs_compress_op {
> >  			  unsigned long start_byte,
> >  			  size_t srclen, size_t destlen);
> >  
> > -	void (*set_level)(struct list_head *ws, unsigned int type);
> > +	unsigned int (*set_level)(unsigned int level);
> 
> It might be good to document the return value since this is an
> interface. AFAICS implementations are required to return the actual
> level set irrespective of what level was passed in, no?
> 
> >  };

Yeah that makes sense. I've added a comment about it in
btrfs_compress_op.

> >  
> >  static void zlib_put_workspace(struct list_head *ws)
> > @@ -413,15 +418,14 @@ static int zlib_decompress(struct list_head *ws, unsigned char *data_in,
> >  	return ret;
> >  }
> >  
> > -static void zlib_set_level(struct list_head *ws, unsigned int type)
> > +static unsigned int zlib_set_level(unsigned int level)
> >  {
> > -	struct workspace *workspace = list_entry(ws, struct workspace, list);
> > -	unsigned int level = BTRFS_COMPRESS_LEVEL(type);
> > -
> > -	if (level > 9)
> > +	if (!level)
> > +		level = BTRFS_ZLIB_DEFAULT_LEVEL;
> > +	else if (level > 9)
> >  		level = 9;
> 
> nit:  This makes it a bit more obvious (IMO) that you are essentially
> doing max:
> 
> if (!level)
>    level = BTRFS_ZLIB_DEFAULT_LEVEL;
> level = max(level, 9);
> 

I agree. I've updated it in both places. It should be min instead of max
though.

Thanks,
Dennis