Re: [PATCH 03/19] mkfs: Sanitise the superblock feature macros

[Eric Sandeen <sandeen@sandeen.net>]

On 3/24/16 6:15 AM, jtulak@redhat.com wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> UPDATE
> o disable finobt when crc=0 no matter if user used -m finobt=X
> o split line > 80 chars
> o remove unused variable
> o add omitted finobtflag
> o change variables in spinodes case to look like surrounding code
> o add I_ALIGN reqval

Just FYI - generally, the patch changelog goes below the "---"
so it doesn't end up as part of the changelog in git.

...

(sorry for being late to the review party btw...)

>  	} else {
>  		/*
>  		 * The kernel doesn't currently support crc=0,finobt=1
> -		 * filesystems. If crcs are not enabled and the user has
> -		 * explicitly turned them off then silently turn them off
> -		 * to avoid an unnecessary warning. If the user explicitly
> -		 * tried to use crc=0,finobt=1, then issue a warning before
> -		 * turning them off.
> +		 * filesystems. If crcs are not enabled and the user has not
> +		 * explicitly turned finobt on, then silently turn it off to
> +		 * avoid an unnecessary warning. If the user explicitly tried
> +		 * to use crc=0,finobt=1, then issue a warning before turning
> +		 * them off.
>  		 */
> -		if (finobt && finobtflag) {
> -			fprintf(stderr,
> -_("warning: finobt not supported without CRC support, disabled.\n"));
> +		if (sb_feat.finobt){
> +			if (sb_feat.finobtflag) {
> +				fprintf(stderr,
> +	_("warning: finobt not supported without CRC support, disabled.\n"));
> +			}
> +			sb_feat.finobt = 0;
>  		}
> -		finobt = 0;
>  	}

Is there any other case in mkfs where options are automatically disabled?
I don't think so .. I'd just prefer a failure here, not a fix-up, even
with the warning.  But I guess that's how it was before, so probably
not something to change in this patch.  So never mind.  :)

But, do we need the extra indentation?

		if (sb_feat.finobt && sb_feat.finobtflag) {
			fprintf(stderr,
_("warning: finobt not supported without CRC support, disabled.\n"));
		}
		sb_feat.finobt = 0;

would suffice as before, no? Meh.  Not a big deal I guess....

> @@ -2962,7 +3038,7 @@ _("size %s specified for log subvolume is too large, maximum is %lld blocks\n"),
>  		/*
>  		 * Free INO btree root block
>  		 */
> -		if (!finobt) {
> +		if (!sb_feat.finobt){
                                    ^ please fix whitespace :)

>  			xfs_perag_put(pag);
>  			continue;
>  		}
> 

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs