On Wed, Nov 11, 2015 at 8:43 AM, Barret Rhoden <[email protected]> wrote:
> Please run checkpatch on your patches before sending them in. A bunch
> of these are false positives, but many are not.
>
Let me take a look ...
my checkpatch output:
>
> --------------------------------------------------------------------------
> ../patches/0001-Completely-restructured-profiler-code-cutting-all-th.patch
> --------------------------------------------------------------------------
> ERROR: space required after that ',' (ctx:VxV)
> #86: FILE: kern/drivers/dev/kprof.c:406:
> + n = profiler_read(va,n);
>
Final code (tip of HEAD) does not have that anymore.
> ^
>
> ERROR: space required after that ',' (ctx:VxV)
> #161: FILE: kern/include/profiler.h:15:
> +int profiler_read(void *va,int);
>
Ditto.
> ^
>
> ERROR: space required before the open parenthesis '('
> #1233: FILE: kern/src/profiler.c:202:
> + for(core = 0; core < num_cores; core++) {
>
Ditto.
ERROR: do not initialise statics to 0 or NULL
> #41: FILE: kern/src/profiler.c:33:
> +static int profiler_users = 0;
>
Fixing now.
WARNING: kfree(NULL) is safe and this check is probably not required
> #108: FILE: kern/lib/circular_buffer.c:33:
> + if (cb->mem)
> + kfree(cb->mem);
>
Existing code, not mine, that now is gone, BTW.
#320: FILE: kern/drivers/dev/kprof.c:56:
> + {".", {Kprofdirqid, 0, QTDIR},0,
> DMDIR|0550},
> ^
>
Existing code, fixing now.
>
> WARNING: externs should be avoided in .c files
> #329: FILE: kern/drivers/dev/kprof.c:65:
> +extern int booting;
>
Yes, this is an existing one, I can clean up, but please, do not hold this
CRs any further for stuff like this.
I will post a follow up patches for things like these.
>
> WARNING: kfree(NULL) is safe and this check is probably not required
> #413: FILE: kern/drivers/dev/kprof.c:132:
> + if (kprof.pdata) {
> + kfree(kprof.pdata);
>
It does not only kfree, but anyway, fixing this as well.
>
> WARNING: Missing a blank line after declarations
> #546: FILE: kern/drivers/dev/kprof.c:211:
> + int i;
> + ERRSTACK(1);
>
False positive. BTW, empty like after var decls is great for me (my
standard), but 99% of existing code place absolutely no spacing, not only
between vars and code, but also between logical groups on instructions.
>
> ERROR: spaces required around that ':' (ctx:VxW)
> #615: FILE: kern/drivers/dev/kprof.c:264:
> + return kprof.pdata != NULL ? kprof.psize: profiler_size();
^
>
OK, fixing.
>
> ERROR: space required before the open brace '{'
> #646: FILE: kern/drivers/dev/kprof.c:291:
> + if (c->qid.type & QTDIR){
>
> ERROR: "foo* const * bar" should be "foo * const *bar"
> #816: FILE: kern/drivers/dev/kprof.c:420:
> + const char* const * cmds = profiler_configure_cmds();
>
Fixing.
>
> ERROR: Macros with complex values should be enclosed in parentheses
> #1191: FILE: kern/include/kdebug.h:51:
> +#define trace_printx(args...) if (printx_on) trace_printk(TRUE, args)
>
Existing code, fixing now.
>
> ERROR: "foo* const *bar" should be "foo * const *bar"
> #1242: FILE: kern/include/profiler.h:17:
> +const char* const *profiler_configure_cmds(void);
>
Fixing.
>
> WARNING: __packed is preferred over __attribute__((packed))
> #1283: FILE: kern/include/ros/profiler_records.h:17:
> +} __attribute__((packed));
>
ENOENT
ERROR: "foo* bar" should be "foo *bar"
> #1335: FILE: kern/include/string.h:27:
> +void *memchr(const void* mem, int chr, int len);
>
Existing code (a bunch there use the same syntax), fixing that only.
ERROR: do not initialise statics to 0 or NULL
> #1433: FILE: kern/src/profiler.c:40:
> +static int tracing = 0;
>
Dupe, fixing.
>
> ERROR: "foo* bar" should be "foo *bar"
> #1442: FILE: kern/src/profiler.c:50:
> +static inline char* vb_encode_uint64(char* data, uint64_t n)
>
Fixing.
>
> ERROR: "foo* bar" should be "foo *bar"
> #1442: FILE: kern/src/profiler.c:50:
> +static inline char* vb_encode_uint64(char* data, uint64_t n)
>
Dupe.
>
> WARNING: kfree(NULL) is safe and this check is probably not required
> #1743: FILE: kern/src/profiler.c:278:
> + if (profiler_percpu_ctx) {
> + kfree(profiler_percpu_ctx);
>
Fixing...
>
> ERROR: else should follow close brace '}'
> #1812: FILE: kern/src/profiler.c:330:
> + }
> + else
>
Fixing.
>
> ERROR: "foo* const *bar" should be "foo * const *bar"
> #1819: FILE: kern/src/profiler.c:336:
> +const char* const *profiler_configure_cmds(void)
>
Dupe.
>
> ERROR: spaces required around that ':' (ctx:VxW)
> #1887: FILE: kern/src/profiler.c:387:
> + cpu_buf->tracing = onoff ? 1: -1;
>
Fixing ...
> ^
>
> ERROR: spaces required around that ':' (ctx:VxW)
> #2001: FILE: kern/src/profiler.c:448:
> + return profiler_queue ? qlen(profiler_queue): 0;
>
Fixing ...
> ^
>
> ERROR: spaces required around that ':' (ctx:VxW)
> #2007: FILE: kern/src/profiler.c:453:
> + return profiler_queue ? qread(profiler_queue, va, n): 0;
>
Fixing ...
> ^
>
> ERROR: "foo* bar" should be "foo *bar"
> #2036: FILE: kern/src/string.c:121:
> +memchr(const void* mem, int chr, int len)
>
Existing code, fixing ...
>
> total: 16 errors, 8 warnings, 1975 lines checked
>
> ../patches/0013-Implemented-the-new-profiler.patch has style problems,
> please review.
> --------------------------------------------------------------------------
> ../patches/0014-Enabled-prof-kptrace-collection-of-anything-which-go.patch
> --------------------------------------------------------------------------
> WARNING: externs should be avoided in .c files
> #34: FILE: kern/drivers/dev/kprof.c:66:
> +extern system_timing_t system_timing;
>
> ERROR: space required after that ',' (ctx:VxV)
> #64: FILE: kern/include/stdarg.h:13:
> +#define va_copy(d,s) __builtin_va_copy(d,s)
> ^
>
> ERROR: space required after that ',' (ctx:VxV)
> #64: FILE: kern/include/stdarg.h:13:
> +#define va_copy(d,s) __builtin_va_copy(d,s)
>
Following existing code syntax. Changed now.
> ^
>
> total: 2 errors, 1 warnings, 56 lines checked
>
> ../patches/0014-Enabled-prof-kptrace-collection-of-anything-which-go.patch
> has style problems, please review.
> ---------------------------------------------------
> ../patches/0015-Added-kprof-to-perf-converter.patch
> ---------------------------------------------------
> WARNING: line over 80 characters
> #159: FILE: tools/profile/kprof2perf/kprof2perf.c:45:
> + fprintf(stderr, "%s: %d: Assertion failed: " #c
> "\n", __FILE__, __LINE__); \
>
Fixing ...
>
> ERROR: do not initialise statics to 0 or NULL
> #201: FILE: tools/profile/kprof2perf/kprof2perf.c:87:
> +static int debug_level = 0;
>
Fixing ...
>
> ERROR: do not initialise statics to 0 or NULL
> #204: FILE: tools/profile/kprof2perf/kprof2perf.c:90:
> +static struct static_mmap64 *static_mmaps = NULL;
>
Fixing ...
>
> ERROR: "foo* bar" should be "foo *bar"
> #211: FILE: tools/profile/kprof2perf/kprof2perf.c:97:
> +static inline const char* vb_decode_uint64(const char* data, uint64_t
> *pval)
>
> ERROR: "foo* bar" should be "foo *bar"
> #211: FILE: tools/profile/kprof2perf/kprof2perf.c:97:
> +static inline const char* vb_decode_uint64(const char* data, uint64_t
> *pval)
>
Buy one, get two. Fixing now ...
>
> WARNING: line over 80 characters
> #352: FILE: tools/profile/kprof2perf/kprof2perf.c:238:
> +static char *mem_block_write(struct mem_block *mb, const void *data,
> size_t size)
>
Fixing ...
>
> ERROR: spaces required around that ':' (ctx:VxW)
> #373: FILE: tools/profile/kprof2perf/kprof2perf.c:259:
> + return (flags & MBWR_SOLID) ? (space >= size): (space > 0);
>
Fixing ...
> ^
>
> WARNING: line over 80 characters
> #560: FILE: tools/profile/kprof2perf/kprof2perf.c:446:
> + struct proftype_kern_trace64 *rec = (struct proftype_kern_trace64
> *) pr->data;
>
> WARNING: line over 80 characters
> #583: FILE: tools/profile/kprof2perf/kprof2perf.c:469:
> + struct proftype_user_trace64 *rec = (struct proftype_user_trace64
> *) pr->data;
>
Fixing both ...
>
> ERROR: "foo* bar" should be "foo *bar"
> #619: FILE: tools/profile/kprof2perf/kprof2perf.c:505:
> + const uint64_t* ids,
> size_t nids)
>
Fixing ...
>
> WARNING: please, no space before tabs
> #890: FILE: tools/profile/kprof2perf/perf_format.h:85:
> +^I * ^I{ u32^I^I^Ipid, tid; } && PERF_SAMPLE_TID$
>
> WARNING: please, no space before tabs
> #891: FILE: tools/profile/kprof2perf/perf_format.h:86:
> +^I * ^I{ u64^I^I^Itime; } && PERF_SAMPLE_TIME$
>
> WARNING: please, no space before tabs
> #892: FILE: tools/profile/kprof2perf/perf_format.h:87:
> +^I * ^I{ u64^I^I^Iid; } && PERF_SAMPLE_ID$
>
> WARNING: please, no space before tabs
> #893: FILE: tools/profile/kprof2perf/perf_format.h:88:
> +^I * ^I{ u64^I^I^Istream_id;} && PERF_SAMPLE_STREAM_ID$
>
> WARNING: please, no space before tabs
> #894: FILE: tools/profile/kprof2perf/perf_format.h:89:
> +^I * ^I{ u32^I^I^Icpu, res; } && PERF_SAMPLE_CPU$
>
> WARNING: please, no space before tabs
> #924: FILE: tools/profile/kprof2perf/perf_format.h:119:
> +^I * ^Istruct sample_id^I^Isample_id;$
>
> WARNING: please, no space before tabs
> #935: FILE: tools/profile/kprof2perf/perf_format.h:130:
> +^I * ^Istruct sample_id^I^Isample_id;$
>
> WARNING: please, no space before tabs
> #946: FILE: tools/profile/kprof2perf/perf_format.h:141:
> +^I * ^Istruct sample_id^I^Isample_id;$
>
> WARNING: please, no space before tabs
> #957: FILE: tools/profile/kprof2perf/perf_format.h:152:
> +^I * ^Istruct sample_id^I^Isample_id;$
>
> WARNING: please, no space before tabs
> #969: FILE: tools/profile/kprof2perf/perf_format.h:164:
> +^I * ^Istruct sample_id^I^Isample_id;$
>
> WARNING: please, no space before tabs
> #980: FILE: tools/profile/kprof2perf/perf_format.h:175:
> +^I * ^Istruct sample_id^I^Isample_id;$
>
> WARNING: please, no space before tabs
> #1027: FILE: tools/profile/kprof2perf/perf_format.h:222:
> +^I * ^I{ u64^I^I^Iabi; # enum perf_sample_regs_abi$
>
> WARNING: please, no space before tabs
> #1028: FILE: tools/profile/kprof2perf/perf_format.h:223:
> +^I * ^I u64^I^I^Iregs[weight(mask)]; } && PERF_SAMPLE_REGS_USER$
>
> WARNING: please, no space before tabs
> #1030: FILE: tools/profile/kprof2perf/perf_format.h:225:
> +^I * ^I{ u64^I^I^Isize;$
>
> WARNING: please, no space before tabs
> #1031: FILE: tools/profile/kprof2perf/perf_format.h:226:
> +^I * ^I char^I^I^Idata[size];$
>
> WARNING: please, no space before tabs
> #1032: FILE: tools/profile/kprof2perf/perf_format.h:227:
> +^I * ^I u64^I^I^Idyn_size; } && PERF_SAMPLE_STACK_USER$
>
> WARNING: please, no space before tabs
> #1059: FILE: tools/profile/kprof2perf/perf_format.h:254:
> +^I * ^Istruct sample_id^I^Isample_id;$
>
> WARNING: __packed is preferred over __attribute__((packed))
> #1223: FILE: tools/profile/kprof2perf/perf_format.h:418:
> +} __attribute__((packed));
>
> WARNING: __packed is preferred over __attribute__((packed))
> #1249: FILE: tools/profile/kprof2perf/perf_format.h:444:
> +} __attribute__((packed));
>
> WARNING: __packed is preferred over __attribute__((packed))
> #1254: FILE: tools/profile/kprof2perf/perf_format.h:449:
> +} __attribute__((packed));
>
> WARNING: __packed is preferred over __attribute__((packed))
> #1295: FILE: tools/profile/kprof2perf/perf_format.h:490:
> +} __attribute__((packed));
>
> WARNING: __packed is preferred over __attribute__((packed))
> #1307: FILE: tools/profile/kprof2perf/perf_format.h:502:
> +} __attribute__((packed));
>
> WARNING: __packed is preferred over __attribute__((packed))
> #1316: FILE: tools/profile/kprof2perf/perf_format.h:511:
> +} __attribute__((packed));
>
> WARNING: __packed is preferred over __attribute__((packed))
> #1333: FILE: tools/profile/kprof2perf/perf_format.h:528:
> +} __attribute__((packed));
>
> total: 6 errors, 28 warnings, 1291 lines checked
>
This whole bunch coming from Linux. Figure that. Fixed now.
>
> ../patches/0015-Added-kprof-to-perf-converter.patch has style problems,
> please review.
> --------------------------------------------------------------------------
> ../patches/0016-Added-patch-to-Linux-perf-to-allow-it-to-work-with-A.patch
> --------------------------------------------------------------------------
> ERROR: trailing whitespace
> #33: FILE: tools/profile/kprof2perf/perf_patches/perf_patch.diff:14:
> + $
>
> ERROR: trailing whitespace
> #37: FILE: tools/profile/kprof2perf/perf_patches/perf_patch.diff:18:
> + $
>
> ERROR: trailing whitespace
> #45: FILE: tools/profile/kprof2perf/perf_patches/perf_patch.diff:26:
> + $
>
> ERROR: trailing whitespace
> #51: FILE: tools/profile/kprof2perf/perf_patches/perf_patch.diff:32:
> + $
>
> ERROR: trailing whitespace
> #98: FILE: tools/profile/kprof2perf/perf_patches/perf_patch.diff:79:
> + $
>
> ERROR: trailing whitespace
> #102: FILE: tools/profile/kprof2perf/perf_patches/perf_patch.diff:83:
> + $
>
> ERROR: trailing whitespace
> #104: FILE: tools/profile/kprof2perf/perf_patches/perf_patch.diff:85:
> + $
>
> ERROR: trailing whitespace
> #116: FILE: tools/profile/kprof2perf/perf_patches/perf_patch.diff:97:
> + $
>
> ERROR: trailing whitespace
> #127: FILE: tools/profile/kprof2perf/perf_patches/perf_patch.diff:108:
> + $
>
> ERROR: trailing whitespace
> #133: FILE: tools/profile/kprof2perf/perf_patches/perf_patch.diff:114:
> + $
>
> ERROR: trailing whitespace
> #144: FILE: tools/profile/kprof2perf/perf_patches/perf_patch.diff:125:
> + $
>
> ERROR: trailing whitespace
> #149: FILE: tools/profile/kprof2perf/perf_patches/perf_patch.diff:130:
> + $
>
> ERROR: trailing whitespace
> #165: FILE: tools/profile/kprof2perf/perf_patches/perf_patch.diff:146:
> + $
>
> ERROR: trailing whitespace
> #167: FILE: tools/profile/kprof2perf/perf_patches/perf_patch.diff:148:
> + $
>
> ERROR: trailing whitespace
> #173: FILE: tools/profile/kprof2perf/perf_patches/perf_patch.diff:154:
> + $
>
> ERROR: trailing whitespace
> #177: FILE: tools/profile/kprof2perf/perf_patches/perf_patch.diff:158:
> + $
>
> ERROR: trailing whitespace
> #183: FILE: tools/profile/kprof2perf/perf_patches/perf_patch.diff:164:
> + $
>
> ERROR: trailing whitespace
> #188: FILE: tools/profile/kprof2perf/perf_patches/perf_patch.diff:169:
> + $
>
> ERROR: trailing whitespace
> #192: FILE: tools/profile/kprof2perf/perf_patches/perf_patch.diff:173:
> + $
>
> ERROR: trailing whitespace
> #212: FILE: tools/profile/kprof2perf/perf_patches/perf_patch.diff:193:
> + $
>
> total: 20 errors, 0 warnings, 201 lines checked
>
This is a patch file. It is going to fail.
WARNING: please write a paragraph that describes the config symbol fully
> #22: FILE: kern/src/ktest/Kconfig.postboot:120:
> +config TEST_circular_buffer
>
Does not apply.
I will fix those in a follow commit on that branch.
--
You received this message because you are subscribed to the Google Groups
"Akaros" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
To post to this group, send email to [email protected].
For more options, visit https://groups.google.com/d/optout.
