* [PATCH] fscrypt: Reduce object size of logging messages @ 2020-09-04 22:10 Joe Perches 2020-09-04 23:03 ` Eric Biggers 0 siblings, 1 reply; 5+ messages in thread From: Joe Perches @ 2020-09-04 22:10 UTC (permalink / raw) To: Theodore Y. Ts'o, Jaegeuk Kim, Eric Biggers; +Cc: linux-fscrypt, LKML Reduce the object size of logging messages by removing the separate KERN_LEVEL argument and adding it to the format. Miscellanea: o Rename fscypt_msg to fscrypt_printk x86-64 defconfig with fscrypto: Original sizes: $ size fs/crypto/built-in.a -t text data bss dec hex filename 3815 300 24 4139 102b fs/crypto/crypto.o (ex fs/crypto/built-in.a) 4354 84 0 4438 1156 fs/crypto/fname.o (ex fs/crypto/built-in.a) 1484 24 0 1508 5e4 fs/crypto/hkdf.o (ex fs/crypto/built-in.a) 2910 68 0 2978 ba2 fs/crypto/hooks.o (ex fs/crypto/built-in.a) 7797 664 65 8526 214e fs/crypto/keyring.o (ex fs/crypto/built-in.a) 5005 493 0 5498 157a fs/crypto/keysetup.o (ex fs/crypto/built-in.a) 2805 0 544 3349 d15 fs/crypto/keysetup_v1.o (ex fs/crypto/built-in.a) 6391 90 0 6481 1951 fs/crypto/policy.o (ex fs/crypto/built-in.a) 1369 40 0 1409 581 fs/crypto/bio.o (ex fs/crypto/built-in.a) 35930 1763 633 38326 95b6 (TOTALS) New sizes: $ size fs/crypto/built-in.a -t text data bss dec hex filename 3874 300 24 4198 1066 fs/crypto/crypto.o (ex fs/crypto/built-in.a) 4347 84 0 4431 114f fs/crypto/fname.o (ex fs/crypto/built-in.a) 1476 24 0 1500 5dc fs/crypto/hkdf.o (ex fs/crypto/built-in.a) 2902 68 0 2970 b9a fs/crypto/hooks.o (ex fs/crypto/built-in.a) 7781 664 65 8510 213e fs/crypto/keyring.o (ex fs/crypto/built-in.a) 4961 493 0 5454 154e fs/crypto/keysetup.o (ex fs/crypto/built-in.a) 2790 0 544 3334 d06 fs/crypto/keysetup_v1.o (ex fs/crypto/built-in.a) 6306 90 0 6396 18fc fs/crypto/policy.o (ex fs/crypto/built-in.a) 1369 40 0 1409 581 fs/crypto/bio.o (ex fs/crypto/built-in.a) 35806 1763 633 38202 953a (TOTALS) Signed-off-by: Joe Perches <joe@perches.com> --- fs/crypto/crypto.c | 14 ++++++++------ fs/crypto/fscrypt_private.h | 12 ++++++------ 2 files changed, 14 insertions(+), 12 deletions(-) diff --git a/fs/crypto/crypto.c b/fs/crypto/crypto.c index 9212325763b0..c82cc3907e43 100644 --- a/fs/crypto/crypto.c +++ b/fs/crypto/crypto.c @@ -329,25 +329,27 @@ int fscrypt_initialize(unsigned int cop_flags) return err; } -void fscrypt_msg(const struct inode *inode, const char *level, - const char *fmt, ...) +void fscrypt_printk(const struct inode *inode, const char *fmt, ...) { static DEFINE_RATELIMIT_STATE(rs, DEFAULT_RATELIMIT_INTERVAL, DEFAULT_RATELIMIT_BURST); struct va_format vaf; va_list args; + int level; if (!__ratelimit(&rs)) return; va_start(args, fmt); - vaf.fmt = fmt; + level = printk_get_level(fmt); + vaf.fmt = printk_skip_level(fmt); vaf.va = &args; if (inode) - printk("%sfscrypt (%s, inode %lu): %pV\n", - level, inode->i_sb->s_id, inode->i_ino, &vaf); + printk("%c%cfscrypt (%s, inode %lu): %pV\n", + KERN_SOH_ASCII, level, inode->i_sb->s_id, inode->i_ino, + &vaf); else - printk("%sfscrypt: %pV\n", level, &vaf); + printk("%c%cfscrypt: %pV\n", KERN_SOH_ASCII, level, &vaf); va_end(args); } diff --git a/fs/crypto/fscrypt_private.h b/fs/crypto/fscrypt_private.h index 8117a61b6f55..fcdb1fc7cbfd 100644 --- a/fs/crypto/fscrypt_private.h +++ b/fs/crypto/fscrypt_private.h @@ -264,13 +264,13 @@ int fscrypt_crypt_block(const struct inode *inode, fscrypt_direction_t rw, unsigned int offs, gfp_t gfp_flags); struct page *fscrypt_alloc_bounce_page(gfp_t gfp_flags); -void __printf(3, 4) __cold -fscrypt_msg(const struct inode *inode, const char *level, const char *fmt, ...); +void __printf(2, 3) __cold +fscrypt_printk(const struct inode *inode, const char *fmt, ...); -#define fscrypt_warn(inode, fmt, ...) \ - fscrypt_msg((inode), KERN_WARNING, fmt, ##__VA_ARGS__) -#define fscrypt_err(inode, fmt, ...) \ - fscrypt_msg((inode), KERN_ERR, fmt, ##__VA_ARGS__) +#define fscrypt_err(inode, fmt, ...) \ + fscrypt_printk(inode, KERN_ERR fmt, ##__VA_ARGS__) +#define fscrypt_warn(inode, fmt, ...) \ + fscrypt_printk(inode, KERN_WARNING fmt, ##__VA_ARGS__) #define FSCRYPT_MAX_IV_SIZE 32 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] fscrypt: Reduce object size of logging messages 2020-09-04 22:10 [PATCH] fscrypt: Reduce object size of logging messages Joe Perches @ 2020-09-04 23:03 ` Eric Biggers 2020-09-05 4:38 ` Joe Perches 0 siblings, 1 reply; 5+ messages in thread From: Eric Biggers @ 2020-09-04 23:03 UTC (permalink / raw) To: Joe Perches; +Cc: Theodore Y. Ts'o, Jaegeuk Kim, linux-fscrypt, LKML On Fri, Sep 04, 2020 at 03:10:15PM -0700, Joe Perches wrote: > Reduce the object size of logging messages by removing the > separate KERN_LEVEL argument and adding it to the format. > > Miscellanea: > > o Rename fscypt_msg to fscrypt_printk > > x86-64 defconfig with fscrypto: > > Original sizes: > $ size fs/crypto/built-in.a -t > text data bss dec hex filename > 3815 300 24 4139 102b fs/crypto/crypto.o (ex fs/crypto/built-in.a) > 4354 84 0 4438 1156 fs/crypto/fname.o (ex fs/crypto/built-in.a) > 1484 24 0 1508 5e4 fs/crypto/hkdf.o (ex fs/crypto/built-in.a) > 2910 68 0 2978 ba2 fs/crypto/hooks.o (ex fs/crypto/built-in.a) > 7797 664 65 8526 214e fs/crypto/keyring.o (ex fs/crypto/built-in.a) > 5005 493 0 5498 157a fs/crypto/keysetup.o (ex fs/crypto/built-in.a) > 2805 0 544 3349 d15 fs/crypto/keysetup_v1.o (ex fs/crypto/built-in.a) > 6391 90 0 6481 1951 fs/crypto/policy.o (ex fs/crypto/built-in.a) > 1369 40 0 1409 581 fs/crypto/bio.o (ex fs/crypto/built-in.a) > 35930 1763 633 38326 95b6 (TOTALS) > > New sizes: > $ size fs/crypto/built-in.a -t > text data bss dec hex filename > 3874 300 24 4198 1066 fs/crypto/crypto.o (ex fs/crypto/built-in.a) > 4347 84 0 4431 114f fs/crypto/fname.o (ex fs/crypto/built-in.a) > 1476 24 0 1500 5dc fs/crypto/hkdf.o (ex fs/crypto/built-in.a) > 2902 68 0 2970 b9a fs/crypto/hooks.o (ex fs/crypto/built-in.a) > 7781 664 65 8510 213e fs/crypto/keyring.o (ex fs/crypto/built-in.a) > 4961 493 0 5454 154e fs/crypto/keysetup.o (ex fs/crypto/built-in.a) > 2790 0 544 3334 d06 fs/crypto/keysetup_v1.o (ex fs/crypto/built-in.a) > 6306 90 0 6396 18fc fs/crypto/policy.o (ex fs/crypto/built-in.a) > 1369 40 0 1409 581 fs/crypto/bio.o (ex fs/crypto/built-in.a) > 35806 1763 633 38202 953a (TOTALS) > > Signed-off-by: Joe Perches <joe@perches.com> > --- > fs/crypto/crypto.c | 14 ++++++++------ > fs/crypto/fscrypt_private.h | 12 ++++++------ > 2 files changed, 14 insertions(+), 12 deletions(-) > > diff --git a/fs/crypto/crypto.c b/fs/crypto/crypto.c > index 9212325763b0..c82cc3907e43 100644 > --- a/fs/crypto/crypto.c > +++ b/fs/crypto/crypto.c > @@ -329,25 +329,27 @@ int fscrypt_initialize(unsigned int cop_flags) > return err; > } > > -void fscrypt_msg(const struct inode *inode, const char *level, > - const char *fmt, ...) > +void fscrypt_printk(const struct inode *inode, const char *fmt, ...) > { > static DEFINE_RATELIMIT_STATE(rs, DEFAULT_RATELIMIT_INTERVAL, > DEFAULT_RATELIMIT_BURST); > struct va_format vaf; > va_list args; > + int level; > > if (!__ratelimit(&rs)) > return; > > va_start(args, fmt); > - vaf.fmt = fmt; > + level = printk_get_level(fmt); > + vaf.fmt = printk_skip_level(fmt); > vaf.va = &args; > if (inode) > - printk("%sfscrypt (%s, inode %lu): %pV\n", > - level, inode->i_sb->s_id, inode->i_ino, &vaf); > + printk("%c%cfscrypt (%s, inode %lu): %pV\n", > + KERN_SOH_ASCII, level, inode->i_sb->s_id, inode->i_ino, > + &vaf); > else > - printk("%sfscrypt: %pV\n", level, &vaf); > + printk("%c%cfscrypt: %pV\n", KERN_SOH_ASCII, level, &vaf); > va_end(args); The problem with this approach is that if fscrypt_printk() is called without providing a log level in the format string (which one would assume would work, since printk() allows it), then the real format string will be truncated to just KERN_SOH because 'level' will be 0. Can you find a way to avoid that? > -#define fscrypt_warn(inode, fmt, ...) \ > - fscrypt_msg((inode), KERN_WARNING, fmt, ##__VA_ARGS__) > -#define fscrypt_err(inode, fmt, ...) \ > - fscrypt_msg((inode), KERN_ERR, fmt, ##__VA_ARGS__) > +#define fscrypt_err(inode, fmt, ...) \ > + fscrypt_printk(inode, KERN_ERR fmt, ##__VA_ARGS__) > +#define fscrypt_warn(inode, fmt, ...) \ > + fscrypt_printk(inode, KERN_WARNING fmt, ##__VA_ARGS__) It's probably best to keep the parentheses around 'inode'. - Eric ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] fscrypt: Reduce object size of logging messages 2020-09-04 23:03 ` Eric Biggers @ 2020-09-05 4:38 ` Joe Perches 2020-09-07 22:23 ` Eric Biggers 0 siblings, 1 reply; 5+ messages in thread From: Joe Perches @ 2020-09-05 4:38 UTC (permalink / raw) To: Eric Biggers; +Cc: Theodore Y. Ts'o, Jaegeuk Kim, linux-fscrypt, LKML On Fri, 2020-09-04 at 16:03 -0700, Eric Biggers wrote: > On Fri, Sep 04, 2020 at 03:10:15PM -0700, Joe Perches wrote: > > Reduce the object size of logging messages by removing the > > separate KERN_LEVEL argument and adding it to the format. > > > > Miscellanea: > > > > o Rename fscypt_msg to fscrypt_printk > > > > x86-64 defconfig with fscrypto: > > > > Original sizes: > > $ size fs/crypto/built-in.a -t > > text data bss dec hex filename > > 3815 300 24 4139 102b fs/crypto/crypto.o (ex fs/crypto/built-in.a) > > 4354 84 0 4438 1156 fs/crypto/fname.o (ex fs/crypto/built-in.a) > > 1484 24 0 1508 5e4 fs/crypto/hkdf.o (ex fs/crypto/built-in.a) > > 2910 68 0 2978 ba2 fs/crypto/hooks.o (ex fs/crypto/built-in.a) > > 7797 664 65 8526 214e fs/crypto/keyring.o (ex fs/crypto/built-in.a) > > 5005 493 0 5498 157a fs/crypto/keysetup.o (ex fs/crypto/built-in.a) > > 2805 0 544 3349 d15 fs/crypto/keysetup_v1.o (ex fs/crypto/built-in.a) > > 6391 90 0 6481 1951 fs/crypto/policy.o (ex fs/crypto/built-in.a) > > 1369 40 0 1409 581 fs/crypto/bio.o (ex fs/crypto/built-in.a) > > 35930 1763 633 38326 95b6 (TOTALS) > > > > New sizes: > > $ size fs/crypto/built-in.a -t > > text data bss dec hex filename > > 3874 300 24 4198 1066 fs/crypto/crypto.o (ex fs/crypto/built-in.a) > > 4347 84 0 4431 114f fs/crypto/fname.o (ex fs/crypto/built-in.a) > > 1476 24 0 1500 5dc fs/crypto/hkdf.o (ex fs/crypto/built-in.a) > > 2902 68 0 2970 b9a fs/crypto/hooks.o (ex fs/crypto/built-in.a) > > 7781 664 65 8510 213e fs/crypto/keyring.o (ex fs/crypto/built-in.a) > > 4961 493 0 5454 154e fs/crypto/keysetup.o (ex fs/crypto/built-in.a) > > 2790 0 544 3334 d06 fs/crypto/keysetup_v1.o (ex fs/crypto/built-in.a) > > 6306 90 0 6396 18fc fs/crypto/policy.o (ex fs/crypto/built-in.a) > > 1369 40 0 1409 581 fs/crypto/bio.o (ex fs/crypto/built-in.a) > > 35806 1763 633 38202 953a (TOTALS) > > > > Signed-off-by: Joe Perches <joe@perches.com> > > --- > > fs/crypto/crypto.c | 14 ++++++++------ > > fs/crypto/fscrypt_private.h | 12 ++++++------ > > 2 files changed, 14 insertions(+), 12 deletions(-) > > > > diff --git a/fs/crypto/crypto.c b/fs/crypto/crypto.c > > index 9212325763b0..c82cc3907e43 100644 > > --- a/fs/crypto/crypto.c > > +++ b/fs/crypto/crypto.c > > @@ -329,25 +329,27 @@ int fscrypt_initialize(unsigned int cop_flags) > > return err; > > } > > > > -void fscrypt_msg(const struct inode *inode, const char *level, > > - const char *fmt, ...) > > +void fscrypt_printk(const struct inode *inode, const char *fmt, ...) > > { > > static DEFINE_RATELIMIT_STATE(rs, DEFAULT_RATELIMIT_INTERVAL, > > DEFAULT_RATELIMIT_BURST); > > struct va_format vaf; > > va_list args; > > + int level; > > > > if (!__ratelimit(&rs)) > > return; > > > > va_start(args, fmt); > > - vaf.fmt = fmt; > > + level = printk_get_level(fmt); > > + vaf.fmt = printk_skip_level(fmt); > > vaf.va = &args; > > if (inode) > > - printk("%sfscrypt (%s, inode %lu): %pV\n", > > - level, inode->i_sb->s_id, inode->i_ino, &vaf); > > + printk("%c%cfscrypt (%s, inode %lu): %pV\n", > > + KERN_SOH_ASCII, level, inode->i_sb->s_id, inode->i_ino, > > + &vaf); > > else > > - printk("%sfscrypt: %pV\n", level, &vaf); > > + printk("%c%cfscrypt: %pV\n", KERN_SOH_ASCII, level, &vaf); > > va_end(args); > > The problem with this approach is that if fscrypt_printk() is called without > providing a log level in the format string (which one would assume would work, > since printk() allows it), then the real format string will be truncated to just > KERN_SOH because 'level' will be 0. > Can you find a way to avoid that? While I don't think this is a problem in that all the fscrypt_<level> calls will always prefix a KERN_<LEVEL>, another approach is to use what btrfs uses: char lvl[PRINTK_MAX_SINGLE_HEADER_LEN + 1] = "\0"; ... while ((kern_level = printk_get_level(fmt)) != 0) { size_t size = printk_skip_level(fmt) - fmt; if (kern_level >= '0' && kern_level <= '7') { memcpy(lvl, fmt, size); lvl[size] = '\0'; } fmt += size; } and use "%s...", lvl, ... > > -#define fscrypt_warn(inode, fmt, ...) \ > > - fscrypt_msg((inode), KERN_WARNING, fmt, ##__VA_ARGS__) > > -#define fscrypt_err(inode, fmt, ...) \ > > - fscrypt_msg((inode), KERN_ERR, fmt, ##__VA_ARGS__) > > +#define fscrypt_err(inode, fmt, ...) \ > > + fscrypt_printk(inode, KERN_ERR fmt, ##__VA_ARGS__) > > +#define fscrypt_warn(inode, fmt, ...) \ > > + fscrypt_printk(inode, KERN_WARNING fmt, ##__VA_ARGS__) > > It's probably best to keep the parentheses around 'inode'. Not really as it's an independent argument that can't effectively have any other purpose but to be an argument to the fsrypt_printk function. ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] fscrypt: Reduce object size of logging messages 2020-09-05 4:38 ` Joe Perches @ 2020-09-07 22:23 ` Eric Biggers 2020-09-07 23:00 ` Joe Perches 0 siblings, 1 reply; 5+ messages in thread From: Eric Biggers @ 2020-09-07 22:23 UTC (permalink / raw) To: Joe Perches; +Cc: Theodore Y. Ts'o, Jaegeuk Kim, linux-fscrypt, LKML On Fri, Sep 04, 2020 at 09:38:23PM -0700, Joe Perches wrote: > > > diff --git a/fs/crypto/crypto.c b/fs/crypto/crypto.c > > > index 9212325763b0..c82cc3907e43 100644 > > > --- a/fs/crypto/crypto.c > > > +++ b/fs/crypto/crypto.c > > > @@ -329,25 +329,27 @@ int fscrypt_initialize(unsigned int cop_flags) > > > return err; > > > } > > > > > > -void fscrypt_msg(const struct inode *inode, const char *level, > > > - const char *fmt, ...) > > > +void fscrypt_printk(const struct inode *inode, const char *fmt, ...) > > > { > > > static DEFINE_RATELIMIT_STATE(rs, DEFAULT_RATELIMIT_INTERVAL, > > > DEFAULT_RATELIMIT_BURST); > > > struct va_format vaf; > > > va_list args; > > > + int level; > > > > > > if (!__ratelimit(&rs)) > > > return; > > > > > > va_start(args, fmt); > > > - vaf.fmt = fmt; > > > + level = printk_get_level(fmt); > > > + vaf.fmt = printk_skip_level(fmt); > > > vaf.va = &args; > > > if (inode) > > > - printk("%sfscrypt (%s, inode %lu): %pV\n", > > > - level, inode->i_sb->s_id, inode->i_ino, &vaf); > > > + printk("%c%cfscrypt (%s, inode %lu): %pV\n", > > > + KERN_SOH_ASCII, level, inode->i_sb->s_id, inode->i_ino, > > > + &vaf); > > > else > > > - printk("%sfscrypt: %pV\n", level, &vaf); > > > + printk("%c%cfscrypt: %pV\n", KERN_SOH_ASCII, level, &vaf); > > > va_end(args); > > > > The problem with this approach is that if fscrypt_printk() is called without > > providing a log level in the format string (which one would assume would work, > > since printk() allows it), then the real format string will be truncated to just > > KERN_SOH because 'level' will be 0. > > Can you find a way to avoid that? > > While I don't think this is a problem in that all the fscrypt_<level> > calls will always prefix a KERN_<LEVEL>, It's still a pitfall that people could run into later. It would be better to make fscrypt_printk() work in the expected way. > what btrfs uses: > > char lvl[PRINTK_MAX_SINGLE_HEADER_LEN + 1] = "\0"; > ... > while ((kern_level = printk_get_level(fmt)) != 0) { > size_t size = printk_skip_level(fmt) - fmt; > > if (kern_level >= '0' && kern_level <= '7') { > memcpy(lvl, fmt, size); > lvl[size] = '\0'; > } > fmt += size; > } > > and use "%s...", lvl, ... > Is the loop really needed? How about just: diff --git a/fs/crypto/crypto.c b/fs/crypto/crypto.c index 9212325763b0f..c5a87c2fe1020 100644 --- a/fs/crypto/crypto.c +++ b/fs/crypto/crypto.c @@ -329,11 +329,12 @@ int fscrypt_initialize(unsigned int cop_flags) return err; } -void fscrypt_msg(const struct inode *inode, const char *level, - const char *fmt, ...) +void fscrypt_printk(const struct inode *inode, const char *fmt, ...) { static DEFINE_RATELIMIT_STATE(rs, DEFAULT_RATELIMIT_INTERVAL, DEFAULT_RATELIMIT_BURST); + const char *raw_fmt = printk_skip_level(fmt); + int hdr_len = raw_fmt - fmt; struct va_format vaf; va_list args; @@ -341,13 +342,13 @@ void fscrypt_msg(const struct inode *inode, const char *level, return; va_start(args, fmt); - vaf.fmt = fmt; + vaf.fmt = raw_fmt; vaf.va = &args; if (inode) - printk("%sfscrypt (%s, inode %lu): %pV\n", - level, inode->i_sb->s_id, inode->i_ino, &vaf); + printk("%.*sfscrypt (%s, inode %lu): %pV\n", + hdr_len, fmt, inode->i_sb->s_id, inode->i_ino, &vaf); else - printk("%sfscrypt: %pV\n", level, &vaf); + printk("%.*sfscrypt: %pV\n", hdr_len, fmt, &vaf); va_end(args); } > > > > -#define fscrypt_warn(inode, fmt, ...) \ > > > - fscrypt_msg((inode), KERN_WARNING, fmt, ##__VA_ARGS__) > > > -#define fscrypt_err(inode, fmt, ...) \ > > > - fscrypt_msg((inode), KERN_ERR, fmt, ##__VA_ARGS__) > > > +#define fscrypt_err(inode, fmt, ...) \ > > > + fscrypt_printk(inode, KERN_ERR fmt, ##__VA_ARGS__) > > > +#define fscrypt_warn(inode, fmt, ...) \ > > > + fscrypt_printk(inode, KERN_WARNING fmt, ##__VA_ARGS__) > > > > It's probably best to keep the parentheses around 'inode'. > > Not really as it's an independent argument that can't > effectively have any other purpose but to be an argument > to the fsrypt_printk function. True, but since forgetting to include parentheses around macro arguments is such a common mistake, IMO they should just always be included so that people don't have to think about whether the omission is correct or not. - Eric ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] fscrypt: Reduce object size of logging messages 2020-09-07 22:23 ` Eric Biggers @ 2020-09-07 23:00 ` Joe Perches 0 siblings, 0 replies; 5+ messages in thread From: Joe Perches @ 2020-09-07 23:00 UTC (permalink / raw) To: Eric Biggers; +Cc: Theodore Y. Ts'o, Jaegeuk Kim, linux-fscrypt, LKML Hi again. On Mon, 2020-09-07 at 15:23 -0700, Eric Biggers wrote: > On Fri, Sep 04, 2020 at 09:38:23PM -0700, Joe Perches wrote: > > > > diff --git a/fs/crypto/crypto.c b/fs/crypto/crypto.c > > > > index 9212325763b0..c82cc3907e43 100644 > > > > --- a/fs/crypto/crypto.c > > > > +++ b/fs/crypto/crypto.c > > > > @@ -329,25 +329,27 @@ int fscrypt_initialize(unsigned int cop_flags) > > > > return err; > > > > } > > > > > > > > -void fscrypt_msg(const struct inode *inode, const char *level, > > > > - const char *fmt, ...) > > > > +void fscrypt_printk(const struct inode *inode, const char *fmt, ...) > > > > { > > > > static DEFINE_RATELIMIT_STATE(rs, DEFAULT_RATELIMIT_INTERVAL, > > > > DEFAULT_RATELIMIT_BURST); > > > > struct va_format vaf; > > > > va_list args; > > > > + int level; > > > > > > > > if (!__ratelimit(&rs)) > > > > return; > > > > > > > > va_start(args, fmt); > > > > - vaf.fmt = fmt; > > > > + level = printk_get_level(fmt); > > > > + vaf.fmt = printk_skip_level(fmt); > > > > vaf.va = &args; > > > > if (inode) > > > > - printk("%sfscrypt (%s, inode %lu): %pV\n", > > > > - level, inode->i_sb->s_id, inode->i_ino, &vaf); > > > > + printk("%c%cfscrypt (%s, inode %lu): %pV\n", > > > > + KERN_SOH_ASCII, level, inode->i_sb->s_id, inode->i_ino, > > > > + &vaf); > > > > else > > > > - printk("%sfscrypt: %pV\n", level, &vaf); > > > > + printk("%c%cfscrypt: %pV\n", KERN_SOH_ASCII, level, &vaf); > > > > va_end(args); > > > > > > The problem with this approach is that if fscrypt_printk() is called without > > > providing a log level in the format string (which one would assume would work, > > > since printk() allows it), then the real format string will be truncated to just > > > KERN_SOH because 'level' will be 0. > > > Can you find a way to avoid that? > > > > While I don't think this is a problem in that all the fscrypt_<level> > > calls will always prefix a KERN_<LEVEL>, > > It's still a pitfall that people could run into later. It would be better to > make fscrypt_printk() work in the expected way. > > > what btrfs uses: > > > > char lvl[PRINTK_MAX_SINGLE_HEADER_LEN + 1] = "\0"; > > ... > > while ((kern_level = printk_get_level(fmt)) != 0) { > > size_t size = printk_skip_level(fmt) - fmt; > > > > if (kern_level >= '0' && kern_level <= '7') { > > memcpy(lvl, fmt, size); > > lvl[size] = '\0'; > > } > > fmt += size; > > } > > > > and use "%s...", lvl, ... > > Is the loop really needed? It prevents defects (that btrfs had) where btrfs_<level> used formats with KERN_<LEVEL>. > > > > +#define fscrypt_warn(inode, fmt, ...) \ > > > > + fscrypt_printk(inode, KERN_WARNING fmt, ##__VA_ARGS__) > > > > > > It's probably best to keep the parentheses around 'inode'. > > > > Not really as it's an independent argument that can't > > effectively have any other purpose but to be an argument > > to the fsrypt_printk function. > > True, but since forgetting to include parentheses around macro arguments is such > a common mistake, IMO they should just always be included so that people don't > have to think about whether the omission is correct or not. We think differently. Unnecessary parentheses are unnecessary and there isn't a possiblity adding them here can be useful. ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-09-07 23:01 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-09-04 22:10 [PATCH] fscrypt: Reduce object size of logging messages Joe Perches 2020-09-04 23:03 ` Eric Biggers 2020-09-05 4:38 ` Joe Perches 2020-09-07 22:23 ` Eric Biggers 2020-09-07 23:00 ` Joe Perches
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).