| Message ID | 1441619584-17992-4-git-send-email-pbonzini@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Mon, Sep 07, 2015 at 11:53:03AM +0200, Paolo Bonzini wrote: > Mostly change severity levels, but some tests can also be adjusted to refer > to QEMU APIs or data structures. > > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > scripts/checkpatch.pl | 141 +++++++++++++++++++++----------------------------- > 1 file changed, 60 insertions(+), 81 deletions(-) Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
On 7 September 2015 at 10:53, Paolo Bonzini <pbonzini@redhat.com> wrote: > Mostly change severity levels, but some tests can also be adjusted to refer > to QEMU APIs or data structures. > > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > @@ -1956,9 +1941,14 @@ sub process { > ERROR("open brace '{' following $1 go on the same line\n" . $hereprev); > } > > +# ... however, open braces on typedef lines should be avoided. > + if ($line =~ /^.\s*typedef\s+(enum|union|struct)(?:\s+$Ident\b)?.*[^;]$/) { > + ERROR("typedefs should be separate from struct declaration\n" . $herecurr); > + } > + > # missing space after union, struct or enum definition Can we revert this one, please? Checkpatch now warns about constructs like typedef struct MyDevice { DeviceState parent; int reg0, reg1, reg2; } MyDevice; which I think are common practice throughout QEMU (recommended as part of the examples in include/qom/object.h and in http://wiki.qemu.org/QOMConventions, for instance). $ git grep 'typedef struct.*{' |wc -l 1884 thanks -- PMM
On 17/09/2015 16:24, Peter Maydell wrote: > Can we revert this one, please? Checkpatch now warns about constructs > like > typedef struct MyDevice { > DeviceState parent; > > int reg0, reg1, reg2; > } MyDevice; It's interesting that qom/object.h documents this and start like: typedef struct ObjectClass ObjectClass; typedef struct Object Object; typedef struct TypeInfo TypeInfo; typedef struct InterfaceClass InterfaceClass; typedef struct InterfaceInfo InterfaceInfo; I have a patch to flag widely-disrespected rules that we still want to encourage in patches. Would you agree with filing these typedefs under this category? Paolo
On 17 September 2015 at 17:00, Paolo Bonzini <pbonzini@redhat.com> wrote: > > > On 17/09/2015 16:24, Peter Maydell wrote: >> Can we revert this one, please? Checkpatch now warns about constructs >> like >> typedef struct MyDevice { >> DeviceState parent; >> >> int reg0, reg1, reg2; >> } MyDevice; > > It's interesting that qom/object.h documents this and start like: > > typedef struct ObjectClass ObjectClass; > typedef struct Object Object; > > typedef struct TypeInfo TypeInfo; > > typedef struct InterfaceClass InterfaceClass; > typedef struct InterfaceInfo InterfaceInfo; > > I have a patch to flag widely-disrespected rules that we still want to > encourage in patches. Would you agree with filing these typedefs under > this category? No, I think that having a separate typedef is worse. The only exceptions are (a) when you need it to be separate because you need to use the type within the struct itself (or some similar dependency loop) (b) when you want to put the typedef in include/qemu/typedefs.h. I really don't see any need to suddenly outlaw something that's been accepted as standard good QEMU style for a long time. (You could make checkpatch warn about typedef struct Foo { stuff; } Foo; if you like, the newline before the '{' is out of line with our usual approach.) thanks -- PMM
On 17/09/2015 18:16, Peter Maydell wrote: > On 17 September 2015 at 17:00, Paolo Bonzini <pbonzini@redhat.com> wrote: >> >> >> On 17/09/2015 16:24, Peter Maydell wrote: >>> Can we revert this one, please? Checkpatch now warns about constructs >>> like >>> typedef struct MyDevice { >>> DeviceState parent; >>> >>> int reg0, reg1, reg2; >>> } MyDevice; >> >> It's interesting that qom/object.h documents this and start like: >> >> typedef struct ObjectClass ObjectClass; >> typedef struct Object Object; >> >> typedef struct TypeInfo TypeInfo; >> >> typedef struct InterfaceClass InterfaceClass; >> typedef struct InterfaceInfo InterfaceInfo; >> >> I have a patch to flag widely-disrespected rules that we still want to >> encourage in patches. Would you agree with filing these typedefs under >> this category? > > No, I think that having a separate typedef is worse. The > only exceptions are (a) when you need it to be separate because > you need to use the type within the struct itself (or some > similar dependency loop) (b) when you want to put the typedef > in include/qemu/typedefs.h. > > I really don't see any need to suddenly outlaw something > that's been accepted as standard good QEMU style for a > long time. I think it varies depending on the maintainer. PPC, USB, SCSI, ACPI all use a separate typedef. I'll prepare a revert. Paolo
On 09/17/2015 10:32 AM, Paolo Bonzini wrote: >>>> Can we revert this one, please? Checkpatch now warns about constructs >>>> like >>>> typedef struct MyDevice { >>>> DeviceState parent; >>>> >>>> int reg0, reg1, reg2; >>>> } MyDevice; >>> >>> It's interesting that qom/object.h documents this and start like: >>> >>> typedef struct ObjectClass ObjectClass; > I think it varies depending on the maintainer. PPC, USB, SCSI, ACPI all > use a separate typedef. I'll prepare a revert. And qapi is about to switch from inline to separate: https://lists.gnu.org/archive/html/qemu-devel/2015-09/msg04291.html
Eric Blake <eblake@redhat.com> writes: > On 09/17/2015 10:32 AM, Paolo Bonzini wrote: > >>>>> Can we revert this one, please? Checkpatch now warns about constructs >>>>> like >>>>> typedef struct MyDevice { >>>>> DeviceState parent; >>>>> >>>>> int reg0, reg1, reg2; >>>>> } MyDevice; >>>> >>>> It's interesting that qom/object.h documents this and start like: >>>> >>>> typedef struct ObjectClass ObjectClass; > >> I think it varies depending on the maintainer. PPC, USB, SCSI, ACPI all >> use a separate typedef. I'll prepare a revert. > > And qapi is about to switch from inline to separate: > > https://lists.gnu.org/archive/html/qemu-devel/2015-09/msg04291.html For a technical reason: DRY in the generator source code. For some types, we need the typedef name declared in one place, and the struct defined in another. Instead of having two variants, one for separate typedef / struct and one for combined, plus the logic to decide which one to use, we simply generate separate always.
On 17 September 2015 at 17:32, Paolo Bonzini <pbonzini@redhat.com> wrote: > > > On 17/09/2015 18:16, Peter Maydell wrote: >> On 17 September 2015 at 17:00, Paolo Bonzini <pbonzini@redhat.com> wrote: >>> >>> >>> On 17/09/2015 16:24, Peter Maydell wrote: >>>> Can we revert this one, please? Checkpatch now warns about constructs >>>> like >>>> typedef struct MyDevice { >>>> DeviceState parent; >>>> >>>> int reg0, reg1, reg2; >>>> } MyDevice; > I think it varies depending on the maintainer. PPC, USB, SCSI, ACPI all > use a separate typedef. I'll prepare a revert. Ping on that revert patch? I can't find it onlist... thanks -- PMM
On Sun, Oct 04, 2015 at 07:44:29PM +0100, Peter Maydell wrote: > On 17 September 2015 at 17:32, Paolo Bonzini <pbonzini@redhat.com> wrote: > > > > > > On 17/09/2015 18:16, Peter Maydell wrote: > >> On 17 September 2015 at 17:00, Paolo Bonzini <pbonzini@redhat.com> wrote: > >>> > >>> > >>> On 17/09/2015 16:24, Peter Maydell wrote: > >>>> Can we revert this one, please? Checkpatch now warns about constructs > >>>> like > >>>> typedef struct MyDevice { > >>>> DeviceState parent; > >>>> > >>>> int reg0, reg1, reg2; > >>>> } MyDevice; > > > I think it varies depending on the maintainer. PPC, USB, SCSI, ACPI all > > use a separate typedef. I'll prepare a revert. > > Ping on that revert patch? I can't find it onlist... The x86 pull request I sent today triggers this error, BTW. I hope it won't make the pull request be automatically rejected.
On 5 October 2015 at 19:11, Eduardo Habkost <ehabkost@redhat.com> wrote: > On Sun, Oct 04, 2015 at 07:44:29PM +0100, Peter Maydell wrote: >> On 17 September 2015 at 17:32, Paolo Bonzini <pbonzini@redhat.com> wrote: >> > >> > >> > On 17/09/2015 18:16, Peter Maydell wrote: >> >> On 17 September 2015 at 17:00, Paolo Bonzini <pbonzini@redhat.com> wrote: >> >>> >> >>> >> >>> On 17/09/2015 16:24, Peter Maydell wrote: >> >>>> Can we revert this one, please? Checkpatch now warns about constructs >> >>>> like >> >>>> typedef struct MyDevice { >> >>>> DeviceState parent; >> >>>> >> >>>> int reg0, reg1, reg2; >> >>>> } MyDevice; >> >> > I think it varies depending on the maintainer. PPC, USB, SCSI, ACPI all >> > use a separate typedef. I'll prepare a revert. >> >> Ping on that revert patch? I can't find it onlist... > > The x86 pull request I sent today triggers this error, BTW. I hope it > won't make the pull request be automatically rejected. Nope, because I don't run checkpatch on pull requests. -- PMM
On 05/10/2015 20:47, Peter Maydell wrote: > On 5 October 2015 at 19:11, Eduardo Habkost <ehabkost@redhat.com> wrote: >> On Sun, Oct 04, 2015 at 07:44:29PM +0100, Peter Maydell wrote: >>> On 17 September 2015 at 17:32, Paolo Bonzini <pbonzini@redhat.com> wrote: >>>> >>>> >>>> On 17/09/2015 18:16, Peter Maydell wrote: >>>>> On 17 September 2015 at 17:00, Paolo Bonzini <pbonzini@redhat.com> wrote: >>>>>> >>>>>> >>>>>> On 17/09/2015 16:24, Peter Maydell wrote: >>>>>>> Can we revert this one, please? Checkpatch now warns about constructs >>>>>>> like >>>>>>> typedef struct MyDevice { >>>>>>> DeviceState parent; >>>>>>> >>>>>>> int reg0, reg1, reg2; >>>>>>> } MyDevice; >>> >>>> I think it varies depending on the maintainer. PPC, USB, SCSI, ACPI all >>>> use a separate typedef. I'll prepare a revert. >>> >>> Ping on that revert patch? I can't find it onlist... >> >> The x86 pull request I sent today triggers this error, BTW. I hope it >> won't make the pull request be automatically rejected. > > Nope, because I don't run checkpatch on pull requests. I've sent the patch today, by the way. Sorry for the delay. Paolo
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl index bc32d8f..f234c2f 100755 --- a/scripts/checkpatch.pl +++ b/scripts/checkpatch.pl @@ -152,33 +152,18 @@ our $Sparse = qr{ }x; # Notes to $Attribute: -# We need \b after 'init' otherwise 'initconst' will cause a false positive in a check our $Attribute = qr{ const| - __percpu| - __nocast| - __safe| - __bitwise__| - __packed__| - __packed2__| - __naked| - __maybe_unused| - __always_unused| - __noreturn| - __used| - __cold| - __noclone| - __deprecated| - __read_mostly| - __kprobes| - __(?:mem|cpu|dev|)(?:initdata|initconst|init\b)| - ____cacheline_aligned| - ____cacheline_aligned_in_smp| - ____cacheline_internodealigned_in_smp| - __weak + volatile| + QEMU_NORETURN| + QEMU_WARN_UNUSED_RESULT| + QEMU_SENTINEL| + QEMU_ARTIFICIAL| + QEMU_PACKED| + GCC_FMT_ATTR }x; our $Modifier; -our $Inline = qr{inline|__always_inline|noinline}; +our $Inline = qr{inline}; our $Member = qr{->$Ident|\.$Ident|\[[^]]*\]}; our $Lval = qr{$Ident(?:$Member)*}; @@ -1367,7 +1352,7 @@ sub process { # Check for incorrect file permissions if ($line =~ /^new (file )?mode.*[7531]\d{0,2}$/) { my $permhere = $here . "FILE: $realfile\n"; - if ($realfile =~ /(Makefile|Kconfig|\.c|\.cpp|\.h|\.S|\.tmpl)$/) { + if ($realfile =~ /(\bMakefile(?:\.objs)?|\.c|\.cc|\.cpp|\.h|\.mak|\.[sS])$/) { ERROR("do not set execute permissions for source files\n" . $permhere); } } @@ -1956,9 +1941,14 @@ sub process { ERROR("open brace '{' following $1 go on the same line\n" . $hereprev); } +# ... however, open braces on typedef lines should be avoided. + if ($line =~ /^.\s*typedef\s+(enum|union|struct)(?:\s+$Ident\b)?.*[^;]$/) { + ERROR("typedefs should be separate from struct declaration\n" . $herecurr); + } + # missing space after union, struct or enum definition if ($line =~ /^.\s*(?:typedef\s+)?(enum|union|struct)(?:\s+$Ident)?(?:\s+$Ident)?[=\{]/) { - WARN("missing space after $1 definition\n" . $herecurr); + ERROR("missing space after $1 definition\n" . $herecurr); } # check for spacing round square brackets; allowed: @@ -2276,7 +2266,7 @@ sub process { if ($line =~ /^.\s*return\s*(E[A-Z]*)\s*;/) { my $name = $1; if ($name ne 'EOF' && $name ne 'ERROR') { - CHK("return of an errno should typically be -ve (return -$1)\n" . $herecurr); + WARN("return of an errno should typically be -ve (return -$1)\n" . $herecurr); } } @@ -2680,15 +2670,15 @@ sub process { # warn about #if 0 if ($line =~ /^.\s*\#\s*if\s+0\b/) { - CHK("if this code is redundant consider removing it\n" . + WARN("if this code is redundant consider removing it\n" . $herecurr); } -# check for needless kfree() checks +# check for needless g_free() checks if ($prevline =~ /\bif\s*\(([^\)]*)\)/) { my $expr = $1; - if ($line =~ /\bkfree\(\Q$expr\E\);/) { - WARN("kfree(NULL) is safe this check is probably not required\n" . $hereprev); + if ($line =~ /\bg_free\(\Q$expr\E\);/) { + WARN("g_free(NULL) is safe this check is probably not required\n" . $hereprev); } } # check for needless usb_free_urb() checks @@ -2735,14 +2725,16 @@ sub process { } } # check for memory barriers without a comment. - if ($line =~ /\b(mb|rmb|wmb|read_barrier_depends|smp_mb|smp_rmb|smp_wmb|smp_read_barrier_depends)\(/) { + if ($line =~ /\b(smp_mb|smp_rmb|smp_wmb|smp_read_barrier_depends)\(/) { if (!ctx_has_comment($first_line, $linenr)) { - CHK("memory barrier without comment\n" . $herecurr); + WARN("memory barrier without comment\n" . $herecurr); } } # check of hardware specific defines - if ($line =~ m@^.\s*\#\s*if.*\b(__i386__|__powerpc64__|__sun__|__s390x__)\b@ && $realfile !~ m@include/asm-@) { - CHK("architecture specific defines should be avoided\n" . $herecurr); +# we have e.g. CONFIG_LINUX and CONFIG_WIN32 for common cases +# where they might be necessary. + if ($line =~ m@^.\s*\#\s*if.*\b__@) { + WARN("architecture specific defines should be avoided\n" . $herecurr); } # Check that the storage class is at the beginning of a declaration @@ -2803,9 +2795,13 @@ sub process { } } -# check for pointless casting of kmalloc return - if ($line =~ /\*\s*\)\s*k[czm]alloc\b/) { - WARN("unnecessary cast may hide bugs, see http://c-faq.com/malloc/mallocnocast.html\n" . $herecurr); +# check for pointless casting of g_malloc return + if ($line =~ /\*\s*\)\s*g_(try)?(m|re)alloc(0?)(_n)?\b/) { + if ($2 == 'm') { + WARN("unnecessary cast may hide bugs, use g_$1new$3 instead\n" . $herecurr); + } else { + WARN("unnecessary cast may hide bugs, use g_$1renew$3 instead\n" . $herecurr); + } } # check for gcc specific __FUNCTION__ @@ -2822,54 +2818,37 @@ sub process { WARN("consider using a completion\n" . $herecurr); } -# recommend strict_strto* over simple_strto* - if ($line =~ /\bsimple_(strto.*?)\s*\(/) { - WARN("consider using strict_$1 in preference to simple_$1\n" . $herecurr); +# recommend qemu_strto* over strto* + if ($line =~ /\b(strto.*?)\s*\(/) { + WARN("consider using qemu_$1 in preference to $1\n" . $herecurr); } -# check for __initcall(), use device_initcall() explicitly please - if ($line =~ /^.\s*__initcall\s*\(/) { - WARN("please use device_initcall() instead of __initcall()\n" . $herecurr); +# check for module_init(), use category-specific init macros explicitly please + if ($line =~ /^module_init\s*\(/) { + WARN("please use block_init(), type_init() etc. instead of module_init()\n" . $herecurr); } # check for various ops structs, ensure they are const. - my $struct_ops = qr{acpi_dock_ops| - address_space_operations| - backlight_ops| - block_device_operations| - dentry_operations| - dev_pm_ops| - dma_map_ops| - extent_io_ops| - file_lock_operations| - file_operations| - hv_ops| - ide_dma_ops| - intel_dvo_dev_ops| - item_operations| - iwl_ops| - kgdb_arch| - kgdb_io| - kset_uevent_ops| - lock_manager_operations| - microcode_ops| - mtrr_ops| - neigh_ops| - nlmsvc_binding| - pci_raw_ops| - pipe_buf_operations| - platform_hibernation_ops| - platform_suspend_ops| - proto_ops| - rpc_pipe_ops| - seq_operations| - snd_ac97_build_ops| - soc_pcmcia_socket_ops| - stacktrace_ops| - sysfs_ops| - tty_operations| - usb_mon_operations| - wd_ops}x; + my $struct_ops = qr{AIOCBInfo| + BdrvActionOps| + BlockDevOps| + BlockJobDriver| + DisplayChangeListenerOps| + GraphicHwOps| + IDEDMAOps| + KVMCapabilityInfo| + MemoryRegionIOMMUOps| + MemoryRegionOps| + MemoryRegionPortio| + QEMUFileOps| + SCSIBusInfo| + SCSIReqOps| + Spice[A-Z][a-zA-Z0-9]*Interface| + TPMDriverOps| + USBDesc[A-Z][a-zA-Z0-9]*| + VhostOps| + VMStateDescription| + VMStateInfo}x; if ($line !~ /\bconst\b/ && - $line =~ /\bstruct\s+($struct_ops)\b/) { + $line =~ /\b($struct_ops)\b/) { WARN("struct $1 should normally be const\n" . $herecurr); }
Mostly change severity levels, but some tests can also be adjusted to refer to QEMU APIs or data structures. Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- scripts/checkpatch.pl | 141 +++++++++++++++++++++----------------------------- 1 file changed, 60 insertions(+), 81 deletions(-)