* Re: [PATCH v2 02/10] fs/ntfs3: Add initialization of super block
[not found] <caddbe41eaef4622aab8bac24934eed1@paragon-software.com>
@ 2020-08-21 17:35 ` Randy Dunlap
2020-08-27 16:04 ` Konstantin Komarov
2020-08-21 19:39 ` Joe Perches
2020-08-23 9:55 ` Pali Rohár
2 siblings, 1 reply; 6+ messages in thread
From: Randy Dunlap @ 2020-08-21 17:35 UTC (permalink / raw)
To: Konstantin Komarov, viro, linux-kernel, linux-fsdevel; +Cc: Pali Rohár
On 8/21/20 9:25 AM, Konstantin Komarov wrote:
> +/* O:BAG:BAD:(A;OICI;FA;;;WD) */
What is that notation, please?
> +const u8 s_dir_security[] __aligned(8) = {
> + 0x01, 0x00, 0x04, 0x80, 0x30, 0x00, 0x00, 0x00, 0x40, 0x00, 0x00, 0x00,
> + 0x00, 0x00, 0x00, 0x00, 0x14, 0x00, 0x00, 0x00, 0x02, 0x00, 0x1C, 0x00,
> + 0x01, 0x00, 0x00, 0x00, 0x00, 0x03, 0x14, 0x00, 0xFF, 0x01, 0x1F, 0x00,
> + 0x01, 0x01, 0x00, 0x00, 0x00, 0x00, 0x00, 0x01, 0x00, 0x00, 0x00, 0x00,
> + 0x01, 0x02, 0x00, 0x00, 0x00, 0x00, 0x00, 0x05, 0x20, 0x00, 0x00, 0x00,
> + 0x20, 0x02, 0x00, 0x00, 0x01, 0x02, 0x00, 0x00, 0x00, 0x00, 0x00, 0x05,
> + 0x20, 0x00, 0x00, 0x00, 0x20, 0x02, 0x00, 0x00,
> +};
> +
> + if (0x10000 * sizeof(short) != inode->i_size) {
> + err = -EINVAL;
> + goto out;
> + }
Please put constants on the right side of compares.
> +MODULE_AUTHOR("Konstantin Komarov");
Drop one space in the name.
thanks.
--
~Randy
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 02/10] fs/ntfs3: Add initialization of super block
[not found] <caddbe41eaef4622aab8bac24934eed1@paragon-software.com>
2020-08-21 17:35 ` [PATCH v2 02/10] fs/ntfs3: Add initialization of super block Randy Dunlap
@ 2020-08-21 19:39 ` Joe Perches
2020-08-27 16:14 ` Konstantin Komarov
2020-08-23 9:55 ` Pali Rohár
2 siblings, 1 reply; 6+ messages in thread
From: Joe Perches @ 2020-08-21 19:39 UTC (permalink / raw)
To: Konstantin Komarov, viro, linux-kernel, linux-fsdevel; +Cc: Pali Rohár
On Fri, 2020-08-21 at 16:25 +0000, Konstantin Komarov wrote:
> Initialization of super block for fs/ntfs3
[]
> diff --git a/fs/ntfs3/super.c b/fs/ntfs3/super.c
[]
> +
> +/**
> + * ntfs_trace() - print preformated ntfs specific messages.
> + */
> +void __ntfs_trace(const struct super_block *sb, const char *level,
> + const char *fmt, ...)
This is a printk mechanism.
I suggest renaming this __ntfs_trace function to ntfs_printk
as there is a naming expectation conflict with the tracing
subsystem.
> +{
> + struct va_format vaf;
> + va_list args;
> +
> + va_start(args, fmt);
> + vaf.fmt = fmt;
> + vaf.va = &args;
> + if (!sb)
> + printk("%sntfs3: %pV", level, &vaf);
> + else
> + printk("%sntfs3: %s: %pV", level, sb->s_id, &vaf);
> + va_end(args);
> +}
Also it would be rather smaller overall object code to
change the macros and uses to embed the KERN_<LEVEL> into
the format and remove the const char *level argument.
Use printk_get_level to retrieve the level from the format.
see fs/f2fs/super.c for an example.
This could be something like the below with a '\n' addition
to the format string to ensure that messages are properly
terminated and cannot be interleaved by other subsystems
content that might be in another simultaneously running
thread starting with KERN_CONT.
void ntfs_printk(const struct super_block *sb, const char *fmt, ...)
{
struct va_format vaf;
va_list args;
int level;
va_start(args, fmt);
level = printk_get_level(fmt);
vaf.fmt = printk_skip_level(fmt);
vaf.va = &args;
if (!sb)
printk("%c%cntfs3: %pV\n",
KERN_SOH_ASCII, level, &vaf);
else
printk("%c%cntfs3: %s: %pV\n",
KERN_SOH_ASCII, level, sbi->sb->s_id, &vaf);
va_end(args);
}
> +
> +/* prints info about inode using dentry case if */
> +void __ntfs_inode_trace(struct inode *inode, const char *level, const char *fmt,
ntfs_inode_printk
> + ...)
> +{
> + struct super_block *sb = inode->i_sb;
> + ntfs_sb_info *sbi = sb->s_fs_info;
> + struct dentry *dentry;
> + const char *name = "?";
> + char buf[48];
> + va_list args;
> + struct va_format vaf;
> +
> + if (!__ratelimit(&sbi->ratelimit))
> + return;
> +
> + dentry = d_find_alias(inode);
> + if (dentry) {
> + spin_lock(&dentry->d_lock);
> + name = (const char *)dentry->d_name.name;
> + } else {
> + snprintf(buf, sizeof(buf), "r=%lx", inode->i_ino);
> + name = buf;
> + }
> +
> + va_start(args, fmt);
> + vaf.fmt = fmt;
> + vaf.va = &args;
> + printk("%s%s on %s: %pV", level, name, sb->s_id, &vaf);
> + va_end(args);
> +
> + if (dentry) {
> + spin_unlock(&dentry->d_lock);
> + dput(dentry);
> + }
> +}
Remove level and use printk_get_level as above.
Format string should use '\n' termination here too.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 02/10] fs/ntfs3: Add initialization of super block
[not found] <caddbe41eaef4622aab8bac24934eed1@paragon-software.com>
2020-08-21 17:35 ` [PATCH v2 02/10] fs/ntfs3: Add initialization of super block Randy Dunlap
2020-08-21 19:39 ` Joe Perches
@ 2020-08-23 9:55 ` Pali Rohár
2020-08-27 16:20 ` Konstantin Komarov
2 siblings, 1 reply; 6+ messages in thread
From: Pali Rohár @ 2020-08-23 9:55 UTC (permalink / raw)
To: Konstantin Komarov; +Cc: viro, linux-kernel, linux-fsdevel
On Friday 21 August 2020 16:25:03 Konstantin Komarov wrote:
> + case Opt_nls:
> + match_strlcpy(nls_name, &args[0], sizeof(nls_name));
> + break;
> +
> + /* unknown option */
> + default:
> + if (!silent)
> + ntfs_error(
> + sb,
> + "Unrecognized mount option \"%s\" or missing value",
> + p);
> + //return -EINVAL;
> + }
> + }
> +
> +out:
> + if (nls_name[0]) {
> + sbi->nls = load_nls(nls_name);
> + if (!sbi->nls) {
> + /* critical ?*/
> + ntfs_error(sb, "failed to load \"%s\"\n", nls_name);
> + //return -EINVAL;
Well, I think it is a fatal error if user supplied NLS encoding cannot
be loaded. If user via mount parameter specify that wants encoding XYZ
and kernel loads different (e.g. default one) then userspace would be
confused as it would expect encoding XYZ.
> + }
> + }
> +
> + if (!sbi->nls) {
> + sbi->nls = load_nls_default();
> + if (!sbi->nls) {
> + /* critical */
> + ntfs_error(sb, "failed to load default nls");
> + return -EINVAL;
> + }
> + }
> +
> + return 0;
> +}
^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [PATCH v2 02/10] fs/ntfs3: Add initialization of super block
2020-08-21 17:35 ` [PATCH v2 02/10] fs/ntfs3: Add initialization of super block Randy Dunlap
@ 2020-08-27 16:04 ` Konstantin Komarov
0 siblings, 0 replies; 6+ messages in thread
From: Konstantin Komarov @ 2020-08-27 16:04 UTC (permalink / raw)
To: Randy Dunlap, viro, linux-kernel, linux-fsdevel; +Cc: Pali Rohár
From: Randy Dunlap <rdunlap@infradead.org>
Sent: Friday, August 21, 2020 8:36 PM
> On 8/21/20 9:25 AM, Konstantin Komarov wrote:
>
>
> > +/* O:BAG:BAD:(A;OICI;FA;;;WD) */
>
> What is that notation, please?
>
Apologies. It's MS's SSDL. We will have it explained a bit more in V3.
> > +const u8 s_dir_security[] __aligned(8) = {
[]
>
>
> > +MODULE_AUTHOR("Konstantin Komarov");
>
> Drop one space in the name.
>
Done, will be posted with V3.
>
> thanks.
> --
> ~Randy
^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [PATCH v2 02/10] fs/ntfs3: Add initialization of super block
2020-08-21 19:39 ` Joe Perches
@ 2020-08-27 16:14 ` Konstantin Komarov
0 siblings, 0 replies; 6+ messages in thread
From: Konstantin Komarov @ 2020-08-27 16:14 UTC (permalink / raw)
To: Joe Perches, viro, linux-kernel, linux-fsdevel; +Cc: Pali Rohár
From: Joe Perches <joe@perches.com>
Sent: Friday, August 21, 2020 10:39 PM
>
> On Fri, 2020-08-21 at 16:25 +0000, Konstantin Komarov wrote:
> > Initialization of super block for fs/ntfs3
> []
> > diff --git a/fs/ntfs3/super.c b/fs/ntfs3/super.c
> []
> > +
> > +/**
> > + * ntfs_trace() - print preformated ntfs specific messages.
> > + */
> > +void __ntfs_trace(const struct super_block *sb, const char *level,
> > + const char *fmt, ...)
>
> This is a printk mechanism.
>
> I suggest renaming this __ntfs_trace function to ntfs_printk
> as there is a naming expectation conflict with the tracing
> subsystem.
>
> > +{
[]
> > + else
> > + printk("%sntfs3: %s: %pV", level, sb->s_id, &vaf);
> > + va_end(args);
> > +}
>
> Also it would be rather smaller overall object code to
> change the macros and uses to embed the KERN_<LEVEL> into
> the format and remove the const char *level argument.
>
> Use printk_get_level to retrieve the level from the format.
>
> see fs/f2fs/super.c for an example.
>
> This could be something like the below with a '\n' addition
> to the format string to ensure that messages are properly
> terminated and cannot be interleaved by other subsystems
> content that might be in another simultaneously running
> thread starting with KERN_CONT.
>
> void ntfs_printk(const struct super_block *sb, const char *fmt, ...)
> {
> struct va_format vaf;
> va_list args;
> int level;
>
> va_start(args, fmt);
>
> level = printk_get_level(fmt);
> vaf.fmt = printk_skip_level(fmt);
> vaf.va = &args;
> if (!sb)
> printk("%c%cntfs3: %pV\n",
> KERN_SOH_ASCII, level, &vaf);
> else
> printk("%c%cntfs3: %s: %pV\n",
> KERN_SOH_ASCII, level, sbi->sb->s_id, &vaf);
>
> va_end(args);
> }
>
> > +
> > +/* prints info about inode using dentry case if */
> > +void __ntfs_inode_trace(struct inode *inode, const char *level, const char *fmt,
>
> ntfs_inode_printk
>
> > + ...)
> > +{
> > + struct super_block *sb = inode->i_sb;
> > + ntfs_sb_info *sbi = sb->s_fs_info;
> > + struct dentry *dentry;
> > + const char *name = "?";
> > + char buf[48];
> > + va_list args;
> > + struct va_format vaf;
> > +
> > + if (!__ratelimit(&sbi->ratelimit))
> > + return;
> > +
> > + dentry = d_find_alias(inode);
> > + if (dentry) {
> > + spin_lock(&dentry->d_lock);
> > + name = (const char *)dentry->d_name.name;
> > + } else {
> > + snprintf(buf, sizeof(buf), "r=%lx", inode->i_ino);
> > + name = buf;
> > + }
> > +
> > + va_start(args, fmt);
> > + vaf.fmt = fmt;
> > + vaf.va = &args;
> > + printk("%s%s on %s: %pV", level, name, sb->s_id, &vaf);
> > + va_end(args);
> > +
> > + if (dentry) {
> > + spin_unlock(&dentry->d_lock);
> > + dput(dentry);
> > + }
> > +}
>
> Remove level and use printk_get_level as above.
> Format string should use '\n' termination here too.
>
Thanks for pointing this out and for your effort with the patch, Joe. We will rework logging in V3 so that it's more compliant with Kernel's approach.
^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [PATCH v2 02/10] fs/ntfs3: Add initialization of super block
2020-08-23 9:55 ` Pali Rohár
@ 2020-08-27 16:20 ` Konstantin Komarov
0 siblings, 0 replies; 6+ messages in thread
From: Konstantin Komarov @ 2020-08-27 16:20 UTC (permalink / raw)
To: Pali Rohár; +Cc: viro, linux-kernel, linux-fsdevel
From: Pali Rohár <pali@kernel.org>
Sent: Sunday, August 23, 2020 12:55 PM
>
> On Friday 21 August 2020 16:25:03 Konstantin Komarov wrote:
> > + case Opt_nls:
> > + match_strlcpy(nls_name, &args[0], sizeof(nls_name));
> > + break;
> > +
> > + /* unknown option */
> > + default:
> > + if (!silent)
> > + ntfs_error(
> > + sb,
> > + "Unrecognized mount option \"%s\" or missing value",
> > + p);
> > + //return -EINVAL;
> > + }
> > + }
> > +
> > +out:
> > + if (nls_name[0]) {
> > + sbi->nls = load_nls(nls_name);
> > + if (!sbi->nls) {
> > + /* critical ?*/
> > + ntfs_error(sb, "failed to load \"%s\"\n", nls_name);
> > + //return -EINVAL;
>
> Well, I think it is a fatal error if user supplied NLS encoding cannot
> be loaded. If user via mount parameter specify that wants encoding XYZ
> and kernel loads different (e.g. default one) then userspace would be
> confused as it would expect encoding XYZ.
>
Agreed. Will be fixed in V3.
> > + }
> > + }
> > +
> > + if (!sbi->nls) {
> > + sbi->nls = load_nls_default();
> > + if (!sbi->nls) {
> > + /* critical */
> > + ntfs_error(sb, "failed to load default nls");
> > + return -EINVAL;
> > + }
> > + }
> > +
> > + return 0;
> > +}
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-08-27 16:20 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <caddbe41eaef4622aab8bac24934eed1@paragon-software.com>
2020-08-21 17:35 ` [PATCH v2 02/10] fs/ntfs3: Add initialization of super block Randy Dunlap
2020-08-27 16:04 ` Konstantin Komarov
2020-08-21 19:39 ` Joe Perches
2020-08-27 16:14 ` Konstantin Komarov
2020-08-23 9:55 ` Pali Rohár
2020-08-27 16:20 ` Konstantin Komarov
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).