Message ID | 1363178421.2031.29.camel@joe-AO722 (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
On Thu, 14 Mar 2013 12:24:37 -0700 Joe Perches <joe@perches.com> wrote: > It's not obvious from reading the macro names that these macros > are for debugging. Convert the names to a single more typical > kernel style cifs_dbg macro. > > cERROR(1, ...) -> cifs_dbg(VFS, ...) > cFYI(1, ...) -> cifs_dbg(FYI, ...) > cFYI(DBG2, ...) -> cifs_dbg(NOISY, ...) > > Move the terminating format newline from the macro to the call site. > > Add CONFIG_CIFS_DEBUG function cifs_vfs_err to emit the > "CIFS VFS: " prefix for VFS messages. > > Size is reduced ~ 1% when CONFIG_CIFS_DEBUG is set (default y) > > $ size fs/cifs/cifs.ko* > text data bss dec hex filename > 265245 2525 132 267902 4167e fs/cifs/cifs.ko.new > 268359 2525 132 271016 422a8 fs/cifs/cifs.ko.old > This all looks like good stuff. I am a bit concerned about mashing all of these cleanups into the same patch though. > Other miscellaneous changes around these conversions: > > o Miscellaneous typo fixes > o Add terminating \n's to almost all formats and remove them > from the macros to be more kernel style like. A few formats > previously had defective \n's > o Remove unnecessary OOM messages as kmalloc() calls dump_stack > o Coalesce formats to make grep easier, > added missing spaces when coalescing formats > o Use %s, __func__ instead of embedded function name > o Removed unnecessary "cifs: " prefixes > o Convert kzalloc with multiply to kcalloc > o Remove unused cifswarn macro >
On Thu, 14 Mar 2013 12:24:37 -0700 Joe Perches <joe@perches.com> wrote: > It's not obvious from reading the macro names that these macros > are for debugging. Convert the names to a single more typical > kernel style cifs_dbg macro. > > cERROR(1, ...) -> cifs_dbg(VFS, ...) > cFYI(1, ...) -> cifs_dbg(FYI, ...) > cFYI(DBG2, ...) -> cifs_dbg(NOISY, ...) > > Move the terminating format newline from the macro to the call site. > > Add CONFIG_CIFS_DEBUG function cifs_vfs_err to emit the > "CIFS VFS: " prefix for VFS messages. > > Size is reduced ~ 1% when CONFIG_CIFS_DEBUG is set (default y) > > $ size fs/cifs/cifs.ko* > text data bss dec hex filename > 265245 2525 132 267902 4167e fs/cifs/cifs.ko.new > 268359 2525 132 271016 422a8 fs/cifs/cifs.ko.old > (my apologies -- my MUA has a mind of its own sometimes) This all looks like good stuff. I am a bit concerned about mashing all of these cleanups into the same patch though. > Other miscellaneous changes around these conversions: > > o Miscellaneous typo fixes > o Add terminating \n's to almost all formats and remove them > from the macros to be more kernel style like. A few formats > previously had defective \n's > o Remove unnecessary OOM messages as kmalloc() calls dump_stack > o Coalesce formats to make grep easier, > added missing spaces when coalescing formats > o Use %s, __func__ instead of embedded function name > o Removed unnecessary "cifs: " prefixes > o Convert kzalloc with multiply to kcalloc ^^^ Things like this really ought to be a separate patch, even though it is a trivial change. That's a minor nit though... > o Remove unused cifswarn macro > I think we ought to go ahead and take this for 3.10. I do have some minor concern about having to deal with backports of later patches to kernels that don't have these changes, but hey, that's the price of dealing with old kernels. The sooner Steve merges this into his for-next tree, the better. This bound to give us all sorts of merge conflicts for the 3.10 window, so we want to make sure that people know what to base their work on. Acked-by: Jeff Layton <jlayton@redhat.com> -- To unsubscribe from this list: send the line "unsubscribe linux-cifs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, 2013-03-15 at 15:16 -0500, Steve French wrote: > I would like to merge the three we have (one still has to be put in > for-next - the one from Jeff) for for-next in the next three or fouir > days - but I can create another temporary branch for the ones for 3.10 > - but ... I would like to simplify things by merging the other cleanup > patches we already have first. I agree that we are going to have to > let people know to rebase everything for cifs for 3.10 on this big > cleanup patch. Another possibility is to extend the cifs_dbg(type, fmt. ...) types from VFS, FYI, NOISY so that the messages could be filtered/classified better. Also, perhaps it'd be better to use pr_debug instead of KERN_ERR for the VFS messages. -- To unsubscribe from this list: send the line "unsubscribe linux-cifs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Mar 15, 2013 at 2:01 PM, Jeff Layton <jlayton@redhat.com> wrote: > > On Thu, 14 Mar 2013 12:24:37 -0700 > Joe Perches <joe@perches.com> wrote: > > > It's not obvious from reading the macro names that these macros > > are for debugging. Convert the names to a single more typical > > kernel style cifs_dbg macro. > > > > cERROR(1, ...) -> cifs_dbg(VFS, ...) > > cFYI(1, ...) -> cifs_dbg(FYI, ...) > > cFYI(DBG2, ...) -> cifs_dbg(NOISY, ...) > > > > Move the terminating format newline from the macro to the call site. > > > > Add CONFIG_CIFS_DEBUG function cifs_vfs_err to emit the > > "CIFS VFS: " prefix for VFS messages. > > > > Size is reduced ~ 1% when CONFIG_CIFS_DEBUG is set (default y) > > > > $ size fs/cifs/cifs.ko* > > text data bss dec hex filename > > 265245 2525 132 267902 4167e fs/cifs/cifs.ko.new > > 268359 2525 132 271016 422a8 fs/cifs/cifs.ko.old > > > > (my apologies -- my MUA has a mind of its own sometimes) > > This all looks like good stuff. I am a bit concerned about mashing all > of these cleanups into the same patch though. > > > Other miscellaneous changes around these conversions: > > > > o Miscellaneous typo fixes > > o Add terminating \n's to almost all formats and remove them > > from the macros to be more kernel style like. A few formats > > previously had defective \n's > > o Remove unnecessary OOM messages as kmalloc() calls dump_stack > > o Coalesce formats to make grep easier, > > added missing spaces when coalescing formats > > o Use %s, __func__ instead of embedded function name > > o Removed unnecessary "cifs: " prefixes > > > o Convert kzalloc with multiply to kcalloc > ^^^ > Things like this really ought to be a separate patch, even though it is > a trivial change. That's a minor nit though... > > > o Remove unused cifswarn macro > > > > I think we ought to go ahead and take this for 3.10. I do have some > minor concern about having to deal with backports of later patches to > kernels that don't have these changes, but hey, that's the price of > dealing with old kernels. > > The sooner Steve merges this into his for-next tree, the better. This > bound to give us all sorts of merge conflicts for the 3.10 window, so > we want to make sure that people know what to base their work on. I would like to merge the three we have (one still has to be put in for-next - the one from Jeff) for for-next in the next three or fouir days - but I can create another temporary branch for the ones for 3.10 - but ... I would like to simplify things by merging the other cleanup patches we already have first. I agree that we are going to have to let people know to rebase everything for cifs for 3.10 on this big cleanup patch. -- Thanks, Steve -- Thanks, Steve -- To unsubscribe from this list: send the line "unsubscribe linux-cifs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Mar 15, 2013 at 3:43 PM, Joe Perches <joe@perches.com> wrote: > On Fri, 2013-03-15 at 15:16 -0500, Steve French wrote: > >> I would like to merge the three we have (one still has to be put in >> for-next - the one from Jeff) for for-next in the next three or fouir >> days - but I can create another temporary branch for the ones for 3.10 >> - but ... I would like to simplify things by merging the other cleanup >> patches we already have first. I agree that we are going to have to >> let people know to rebase everything for cifs for 3.10 on this big >> cleanup patch. > > Another possibility is to extend the > cifs_dbg(type, fmt. ...) types from > > VFS, FYI, NOISY > > so that the messages could be > filtered/classified better. > > Also, perhaps it'd be better to use > pr_debug instead of KERN_ERR for > the VFS messages. Although it required hand merging, this large patch is now merged into cifs-2.6.git (for-next branch, destined for 3.10) but ... I only merged three obvious other patches (destined for 3.10) before it 4521ba9ca4138f424769f17ae7166ff5ccf08805 fs: cifs: use kmemdup instead of kmalloc + memcpy 41057e5a5d673015fc0d0e587e993d26f1818b1c cifs: replaced kmalloc + memset with kzalloc 8f43465d17982767dd5b3f5d48216ef5c100c274 cifs: ignore the unc= and prefixpath= mount options and I merged one so far after it that Jeff had already rebased ontop of Joe's large patch 457d2157ec2da6f9fa755e16ab740979f581e328 cifs: remove ENOSPC handling in smb_sendv So ... going forward for 3.10 make sure to fixup subsequent new cifs patches ontop Joe's large "cifs: Rename cERROR and cFYI to cifs_dbg" patch or else it will make merging a mess since Joe's patch changes just about very cifs file due to the macro renaming.
diff --git a/fs/cifs/cifs_debug.h b/fs/cifs/cifs_debug.h index 69ae3d3..c99b40f 100644 --- a/fs/cifs/cifs_debug.h +++ b/fs/cifs/cifs_debug.h @@ -25,18 +25,20 @@ void cifs_dump_mem(char *label, void *data, int length); void cifs_dump_detail(void *); void cifs_dump_mids(struct TCP_Server_Info *); -#ifdef CONFIG_CIFS_DEBUG2 -#define DBG2 2 -#else -#define DBG2 0 -#endif extern int traceSMB; /* flag which enables the function below */ void dump_smb(void *, int); #define CIFS_INFO 0x01 #define CIFS_RC 0x02 #define CIFS_TIMER 0x04 +#define VFS 1 +#define FYI 2 extern int cifsFYI; +#ifdef CONFIG_CIFS_DEBUG2 +#define NOISY 4 +#else +#define NOISY 0 +#endif /* * debug ON @@ -44,31 +46,21 @@ extern int cifsFYI; */ #ifdef CONFIG_CIFS_DEBUG -/* information message: e.g., configuration, major event */ -#define cifsfyi(fmt, ...) \ -do { \ - if (cifsFYI & CIFS_INFO) \ - printk(KERN_DEBUG "%s: " fmt "\n", \ - __FILE__, ##__VA_ARGS__); \ -} while (0) - -#define cFYI(set, fmt, ...) \ -do { \ - if (set) \ - cifsfyi(fmt, ##__VA_ARGS__); \ -} while (0) +__printf(1, 2) void cifs_vfs_err(const char *fmt, ...); -#define cifswarn(fmt, ...) \ - printk(KERN_WARNING fmt "\n", ##__VA_ARGS__) - -/* error event message: e.g., i/o error */ -#define cifserror(fmt, ...) \ - printk(KERN_ERR "CIFS VFS: " fmt "\n", ##__VA_ARGS__); \ - -#define cERROR(set, fmt, ...) \ +/* information message: e.g., configuration, major event */ +#define cifs_dbg(type, fmt, ...) \ do { \ - if (set) \ - cifserror(fmt, ##__VA_ARGS__); \ + if (type == FYI) { \ + if (cifsFYI & CIFS_INFO) { \ + printk(KERN_DEBUG "%s: " fmt, \ + __FILE__, ##__VA_ARGS__); \ + } \ + } else if (type == VFS) { \ + cifs_vfs_err(fmt, ##__VA_ARGS__); \ + } else if (type == NOISY && type != 0) { \ + printk(KERN_DEBUG fmt, ##__VA_ARGS__); \ + } \ } while (0) /* @@ -76,27 +68,11 @@ do { \ * --------- */ #else /* _CIFS_DEBUG */ -#define cifsfyi(fmt, ...) \ +#define cifs_dbg(type, fmt, ...) \ do { \ if (0) \ - printk(KERN_DEBUG "%s: " fmt "\n", \ - __FILE__, ##__VA_ARGS__); \ + printk(KERN_DEBUG fmt, ##__VA_ARGS__); \ } while (0) -#define cFYI(set, fmt, ...) \ -do { \ - if (0 && set) \ - cifsfyi(fmt, ##__VA_ARGS__); \ -} while (0) -#define cifserror(fmt, ...) \ -do { \ - if (0) \ - printk(KERN_ERR "CIFS VFS: " fmt "\n", ##__VA_ARGS__); \ -} while (0) -#define cERROR(set, fmt, ...) \ -do { \ - if (0 && set) \ - cifserror(fmt, ##__VA_ARGS__); \ -} while (0) -#endif /* _CIFS_DEBUG */ +#endif #endif /* _H_CIFS_DEBUG */