All of lore.kernel.org
 help / color / mirror / Atom feed
From: gregkh@linuxfoundation.org (Greg KH)
Subject: [PATCH] erofs: surround fault_injection ralted option parsing using CONFIG_EROFS_FAULT_INJECTION
Date: Mon, 10 Sep 2018 17:52:47 +0200	[thread overview]
Message-ID: <20180910155247.GA17537@kroah.com> (raw)
In-Reply-To: <20180910154551.28922-1-cgxu519@gmx.com>

On Mon, Sep 10, 2018@11:45:51PM +0800, Chengguang Xu wrote:
> It's a little bit strange when fault_injection related
> option fail with -EINVAL which was already disabled
> from config, so surround all fault_injection related option
> parsing code using CONFIG_EROFS_FAULT_INJECTION. Meanwhile,
> slightly change warning message to keep consistency with
> option POSIX_ACL and FS_XATTR.
> 
> Signed-off-by: Chengguang Xu <cgxu519 at gmx.com>
> ---
>  drivers/staging/erofs/super.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/staging/erofs/super.c b/drivers/staging/erofs/super.c
> index 1aec509c805f..4004a00d150d 100644
> --- a/drivers/staging/erofs/super.c
> +++ b/drivers/staging/erofs/super.c
> @@ -237,16 +237,18 @@ static int parse_options(struct super_block *sb, char *options)
>  			infoln("noacl options not supported");
>  			break;
>  #endif
> +#ifdef CONFIG_EROFS_FAULT_INJECTION
>  		case Opt_fault_injection:
>  			if (args->from && match_int(args, &arg))
>  				return -EINVAL;
> -#ifdef CONFIG_EROFS_FAULT_INJECTION
>  			erofs_build_fault_attr(EROFS_SB(sb), arg);
>  			set_opt(EROFS_SB(sb), FAULT_INJECTION);
> +			break;
>  #else
> -			infoln("FAULT_INJECTION was not selected");
> -#endif
> +		case Opt_fault_injection:
> +			infoln("FAULT_INJECTION not supported");
>  			break;
> +#endif

Ugh, that's hard to read.  Why not just hide all of this crazyness
behind a single function that handles the #ifdef mess, like it should
be.  Then just always call handle_fault_injection() or whatever you want
to call it.

That makes it easier to read and understand and maintain over time.

I'll take this for now, but that should be something you work on
eventually.  For the other #ifdef options in here as well.

thanks,

greg k-h

  reply	other threads:[~2018-09-10 15:52 UTC|newest]

Thread overview: 3+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-09-10 15:45 [PATCH] erofs: surround fault_injection ralted option parsing using CONFIG_EROFS_FAULT_INJECTION Chengguang Xu
2018-09-10 15:52 ` Greg KH [this message]
2018-09-10 16:37   ` Gao Xiang

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20180910155247.GA17537@kroah.com \
    --to=gregkh@linuxfoundation.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.