Re: [PATCH 1/1] xfsprogs: Fix uninitialized cfg->lsunit

[Allison Collins <allison.henderson@oracle.com>]

On 6/20/19 8:32 AM, Darrick J. Wong wrote:
> On Wed, Jun 19, 2019 at 11:28:57AM -0700, Allison Collins wrote:
>> While investigating another mkfs bug, noticed that cfg->lsunit is sometimes
>> left uninitialized when it should not.  This is because calc_stripe_factors
>> in some cases needs cfg->loginternal to be set first.  This is done in
>> validate_logdev. So move calc_stripe_factors below validate_logdev while
>> parsing configs.
> 
> <grumble> The cfg in main() is not (in a manner easily detectable by
> toolz) uninitialized, it's zero-initialized by default and we haven't
> set cfg->loginternal correctly yet...
> 
> ...what we really need here is enum { FALSE, TRUE, FILENOTFOUND } to
> detect that we're using incorrect garbage data. :P
> 
> (Really, someone should take a closer look at whether or not there are
> other places where we do things like this...)
> 
> Anyway, this does solve a problem, so
> 
> Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
> 
> --D

Alrighty, thx for the review!

Yes, I gave it a quick look over and didn't notice any other out of 
order usage, but it is really difficult to tell when one function has a 
dependency of another without just pouring through it.

Do you mean to add an enum for every member of the cfg?  That would 
work, but I guess the question then is the added complexity worth just 
making sure everything is configured linearly?

Another approach could be to initialize things to some sort of 
uninitialized signature. Like memset(&cfg, 0xFF, sizeof(struct 
mkfs_params), assuming 0xFF would never otherwise be set.  Then other 
subroutines could check to see if the configs they need are not foxed out.

I think both of these solutions have the effect of scattering default 
config handling and asserts all over the place though.  Which is 
something to consider.  I notice there's been a lot of patches to just 
try and organize things.  Technically it shouldn't be needed if we can 
be diligent about making sure every thing flows forward, but I guess 
deciding which of them is the lesser burden is really the question here. 
  :-)

Thoughts?

Allison

> 
>>
>> Signed-off-by: Allison Collins <allison.henderson@oracle.com>
>> ---
>>   mkfs/xfs_mkfs.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
>> index ddb25ec..f4a5e4b 100644
>> --- a/mkfs/xfs_mkfs.c
>> +++ b/mkfs/xfs_mkfs.c
>> @@ -3995,7 +3995,6 @@ main(
>>   	cfg.rtblocks = calc_dev_size(cli.rtsize, &cfg, &ropts, R_SIZE, "rt");
>>   
>>   	validate_rtextsize(&cfg, &cli, &ft);
>> -	calc_stripe_factors(&cfg, &cli, &ft);
>>   
>>   	/*
>>   	 * Open and validate the device configurations
>> @@ -4005,6 +4004,7 @@ main(
>>   	validate_datadev(&cfg, &cli);
>>   	validate_logdev(&cfg, &cli, &logfile);
>>   	validate_rtdev(&cfg, &cli, &rtfile);
>> +	calc_stripe_factors(&cfg, &cli, &ft);
>>   
>>   	/*
>>   	 * At this point when know exactly what size all the devices are,
>> -- 
>> 2.7.4
>>