linux-ext4 - Re: [PATCH 1/2] resize2fs: Add support for lazy itable initialization

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]

Date:	Tue, 1 Feb 2011 20:31:08 +0100 (CET)
From:	Lukas Czerner <lczerner@...hat.com>
To:	Andreas Dilger <adilger@...ger.ca>
cc:	Lukas Czerner <lczerner@...hat.com>, linux-ext4@...r.kernel.org,
	tytso@....EDU
Subject: Re: [PATCH 1/2] resize2fs: Add support for lazy itable
 initialization

On Tue, 1 Feb 2011, Andreas Dilger wrote:

> On 2011-02-01, at 10:14, Lukas Czerner wrote:
> > This commit adds extended options '-E' to the resize2fs code along with
> > the first extended option lazy_itable_init=n.
> 
> Here it says the option is "=n", is it also possible to force this with "=y"?

Oh, no not really. It should be just =0 or =1, it is the same as we have in
mke2fs.

> 
> > +static void parse_extended_opts(int *flags, const char *opts)
> > +{
> > +		if (!strcmp(token, "lazy_itable_init")) {
> > +			int lazy;
> > +			if (arg)
> > +				lazy = strtoul(arg, &p, 0);
> > +			else
> > +				lazy = 1;
> > +			if (lazy)
> > +				*flags |= RESIZE_LAZY_ITABLE_INIT;
> 
> Here it parses the option as "=0" or "=1", not "=n" or "=y".  It looks like "=n" will accidentally return 0 from strtoul(), so that works as expected, but the "=y" option would fail (it would also return 0).

As I mentioned it is not supposed to work with =y or =n, but only with
=0 or =1. So I think it is expected, isn't it ?

> 
> > +	if (r_usage) {
> > +		fprintf(stderr, _("\nBad option(s) specified: %s\n\n"
> > +			"\tlazy_itable_init=<0 to disable, 1 to enable>\n\n"),
> 
> It looks a bit confusing "=<0 to disable", I thought initially that meant any value <= 0 would disable it, though I later see that there is a closing '>'.  I think it is more standard to use "{}" braces for "one of these options must be given".

:) I did not noticed that before, but you're right, I'll change that to
use "{}".

> 
> > @@ -232,6 +290,12 @@ int main (int argc, char ** argv)
> > +	if (access("/sys/fs/ext4/features/lazy_itable_init", R_OK) == 0)
> > +		flags |= RESIZE_LAZY_ITABLE_INIT;
> > +
> > +	if (extended_opts)
> > +		parse_extended_opts(&flags, extended_opts);
> 
> Good that the command-line options override the default value.

Yes, it suppose to be that way.

> 
> > +.B lazy_itable_init\fR[\fB= \fI<0 to disable, 1 to enable>\fR]
> 
> Similarly, this should use "{}" around the options instead of "<>".
> 
> > +If enabled and the uninit_bg feature is enabled, the inode table will
> > +not be fully initialized by
> 
> I would write "not be zeroed out on disk", since it is otherwise unclear
> if "not fully initialized" means that there will be less inodes available
> or some other issues if the inode table is not "fully" initialized.

Right.

> 
> Cheers, Andreas
> 

Thanks!
-Lukas
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html