Received-SPF: pass (google.com: domain of linux-ext4-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Wed, 29 Apr 2020 13:02:07 +0200
From:   Lukas Czerner <lczerner@redhat.com>
To:     Eric Sandeen <sandeen@sandeen.net>
Cc:     linux-ext4@vger.kernel.org, dhowells@redhat.com,
        viro@zeniv.linux.org.uk
Subject: Re: [PATCH v2 05/17] ext4: Allow sb to be NULL in ext4_msg()
Message-ID: <20200429110207.cvvxn4szl7d36a4x@work>
References: <20200428164536.462-1-lczerner@redhat.com>
 <20200428164536.462-6-lczerner@redhat.com>
 <39a487d3-bce3-0290-229a-c49a540ba7de@sandeen.net>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <39a487d3-bce3-0290-229a-c49a540ba7de@sandeen.net>
Sender: linux-ext4-owner@vger.kernel.org
Precedence: bulk

On Tue, Apr 28, 2020 at 09:38:11PM -0500, Eric Sandeen wrote:
> On 4/28/20 11:45 AM, Lukas Czerner wrote:
> > At the parsing phase of mount in the new mount api sb will not be
> > available so allow sb to be NULL in ext4_msg and use that in
> > handle_mount_opt().
> > 
> > Also change return value to appropriate -EINVAL where needed.
> > 
> > Signed-off-by: Lukas Czerner <lczerner@redhat.com>
> > ---
> >  fs/ext4/super.c | 120 +++++++++++++++++++++++++-----------------------
> >  1 file changed, 63 insertions(+), 57 deletions(-)
> > 
> > diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> > index 2c6fea451d7d..2f7e49bfbf71 100644
> > --- a/fs/ext4/super.c
> > +++ b/fs/ext4/super.c
> > @@ -88,7 +88,7 @@ static void ext4_unregister_li_request(struct super_block *sb);
> >  static void ext4_clear_request_list(void);
> >  static struct inode *ext4_get_journal_inode(struct super_block *sb,
> >  					    unsigned int journal_inum);
> > -static int ext4_validate_options(struct super_block *sb);
> > +static int ext4_validate_options(struct fs_context *fc);
> >  
> >  /*
> >   * Lock ordering
> > @@ -754,13 +754,14 @@ void __ext4_msg(struct super_block *sb,
> >  	struct va_format vaf;
> >  	va_list args;
> >  
> > -	if (!___ratelimit(&(EXT4_SB(sb)->s_msg_ratelimit_state), "EXT4-fs"))
> > +	if (sb &&
> > +	    !___ratelimit(&(EXT4_SB(sb)->s_msg_ratelimit_state), "EXT4-fs"))
> >  		return;
> >  
> >  	va_start(args, fmt);
> >  	vaf.fmt = fmt;
> >  	vaf.va = &args;
> > -	printk("%sEXT4-fs (%s): %pV\n", prefix, sb->s_id, &vaf);
> > +	printk("%sEXT4-fs (%s): %pV\n", prefix, sb ? sb->s_id : "n/a", &vaf);
> 
> Tiny nitpick, I might drop "n/a" - I'm not sure that makes anything clearer:
> 
> "EXT4-fs (n/a): message" seems confusing, but maybe that's just me.
> 
> FWIW xfs just removes the s_id print altogether if it's not available, i.e.:
> 
> static void
> __xfs_printk(
>         const char              *level,
>         const struct xfs_mount  *mp,
>         struct va_format        *vaf)
> {
>         if (mp && mp->m_super) {
>                 printk("%sXFS (%s): %pV\n", level, mp->m_super->s_id, vaf);
>                 return;
>         }
>         printk("%sXFS: %pV\n", level, vaf);
> }
> 
> *shrug* up to personal preference I suppose.

I wanted the message to stay more-or-less uniform, but I think you're
right. I'll drop the (n/a).

-Lukas

> 
> Thanks,
> -Eric
>