[Qemu-devel] [PATCH 0/3] tighten scope of accidentally global variables
From: Jim Meyering meyer...@redhat.com I noticed this commit, virtio-pci: add missing 'static' which made this change: -const MemoryRegionPortio virtio_portio[] = { +static const MemoryRegionPortio virtio_portio[] = { and wondered if there were other variables like that. The following command shows that there are: [note that there are probably more: this finds only those for which the variable name appears in only one source file. ] $ for i in $(nm -e *.o|sed -n 's/.* [BCDGRS] //p'); do \ test $(git grep -lw $i|wc -l) = 1 echo $i;done BlockDeviceIoStatus_lookup SpiceQueryMouseMode_lookup qemu_boot_opts qemu_option_rom_opts vmstate_info_scsi_requests xen_xcg The *_lookup names are false positives, since the symbols are actually used from two or more .o files. Here are patches for the others: Jim Meyering (3): xen: remove unused global, xen_xcg scsi: declare vmstate_info_scsi_requests to be static qemu-config: qemu_option_rom_opts, qemu_boot_opts: declare static hw/scsi-bus.c| 2 +- hw/xen_backend.c | 1 - qemu-config.c| 4 ++-- 3 files changed, 3 insertions(+), 4 deletions(-) -- 1.7.10.2.552.gaa3bb87
Re: [Qemu-devel] [PATCH 0/3] tighten scope of accidentally global variables
On Mon, May 21, 2012 at 10:03 AM, Jim Meyering j...@meyering.net wrote: From: Jim Meyering meyer...@redhat.com I noticed this commit, virtio-pci: add missing 'static' which made this change: -const MemoryRegionPortio virtio_portio[] = { +static const MemoryRegionPortio virtio_portio[] = { and wondered if there were other variables like that. The following command shows that there are: [note that there are probably more: this finds only those for which the variable name appears in only one source file. ] Also, only for files at the top level. What about functions? ;-) $ for i in $(nm -e *.o|sed -n 's/.* [BCDGRS] //p'); do \ test $(git grep -lw $i|wc -l) = 1 echo $i;done BlockDeviceIoStatus_lookup SpiceQueryMouseMode_lookup qemu_boot_opts qemu_option_rom_opts vmstate_info_scsi_requests xen_xcg The *_lookup names are false positives, since the symbols are actually used from two or more .o files. Here are patches for the others: Jim Meyering (3): xen: remove unused global, xen_xcg scsi: declare vmstate_info_scsi_requests to be static qemu-config: qemu_option_rom_opts, qemu_boot_opts: declare static hw/scsi-bus.c | 2 +- hw/xen_backend.c | 1 - qemu-config.c | 4 ++-- 3 files changed, 3 insertions(+), 4 deletions(-) -- 1.7.10.2.552.gaa3bb87
Re: [Qemu-devel] [PATCH 0/3] tighten scope of accidentally global variables
Blue Swirl wrote: On Mon, May 21, 2012 at 10:03 AM, Jim Meyering j...@meyering.net wrote: From: Jim Meyering meyer...@redhat.com I noticed this commit, virtio-pci: add missing 'static' which made this change: -const MemoryRegionPortio virtio_portio[] = { +static const MemoryRegionPortio virtio_portio[] = { and wondered if there were other variables like that. The following command shows that there are: [note that there are probably more: this finds only those for which the variable name appears in only one source file. ] Also, only for files at the top level. Thanks. There were so many .o files, I assumed that all were at the top. Searching all .o files, I found many more: $ for i in $(nm -e $(find . -name '*.o')|sed -n 's/.* [BCDGRS] //p'|sort -u);\ do test $(git grep -lw $i|wc -l) = 1 echo $i;done BlockDeviceIoStatus_lookup DEFAULT_ATR SpiceQueryMouseMode_lookup __jit_debug_descriptor alpha_num_opcodes alpha_num_operands alpha_opcodes alpha_operands arg_table backend_enum_table bdrv_sheepdog bfd_mips_num_builtin_opcodes bfd_mips_num_opcodes bonito_state check_cpuid cris_cond15s cris_cores device_configs display_remote enforce_cpuid floatformat_arm_ext_big floatformat_arm_ext_littlebyte_bigword floatformat_i387_ext floatformat_i960_ext floatformat_ia64_quad_big floatformat_ia64_quad_little floatformat_ia64_spill_big floatformat_ia64_spill_little floatformat_ieee_double_big floatformat_ieee_double_little floatformat_ieee_double_littlebyte_bigword floatformat_ieee_single_big floatformat_ieee_single_little floatformat_m68881_ext floatformat_m88110_ext floatformat_m88110_harris_ext fsl_register_prefix fw_boot_order last_mapping_addr last_mapping_sym last_type leon3_generic_machine m68k_numaliases m68k_numopcodes m68k_opcode_aliases m68k_opcodes memory_region_transaction_depth mips_builtin_opcodes mips_fulong2e_machine mips_opcodes no_reboot num_powerpc_operands para_features powerpc_macros powerpc_num_macros powerpc_num_opcodes powerpc_operands pvr_register_prefix qemu_boot_opts qemu_global_mutex qemu_option_rom_opts register_prefix rtas_next s390_virtio_bus_info sh_table special_register_prefix test_image timers_state v9fs_synth_root virtcon_hds vmstate_bmdma_status vmstate_info_scsi_requests vmstate_rxtx_stats xen_platform_ioport xen_xcg What about functions? ;-) I planned to check them separately, as I do in gnulib's sc_tight_scope syntax-check rule: http://git.sv.gnu.org/cgit/gnulib.git/tree/top/maint.mk#n1443
Re: [Qemu-devel] [PATCH 0/3] tighten scope of accidentally global variables
On Mon, May 21, 2012 at 6:10 PM, Jim Meyering j...@meyering.net wrote: Blue Swirl wrote: On Mon, May 21, 2012 at 10:03 AM, Jim Meyering j...@meyering.net wrote: From: Jim Meyering meyer...@redhat.com I noticed this commit, virtio-pci: add missing 'static' which made this change: -const MemoryRegionPortio virtio_portio[] = { +static const MemoryRegionPortio virtio_portio[] = { and wondered if there were other variables like that. The following command shows that there are: [note that there are probably more: this finds only those for which the variable name appears in only one source file. ] Also, only for files at the top level. Thanks. There were so many .o files, I assumed that all were at the top. Searching all .o files, I found many more: $ for i in $(nm -e $(find . -name '*.o')|sed -n 's/.* [BCDGRS] //p'|sort -u);\ do test $(git grep -lw $i|wc -l) = 1 echo $i;done How about just checking the executables (*-softmmu/qemu-system-xyz and linux-user/qemu-xyz)? BlockDeviceIoStatus_lookup DEFAULT_ATR SpiceQueryMouseMode_lookup __jit_debug_descriptor alpha_num_opcodes alpha_num_operands alpha_opcodes alpha_operands arg_table backend_enum_table bdrv_sheepdog bfd_mips_num_builtin_opcodes bfd_mips_num_opcodes bonito_state check_cpuid cris_cond15s cris_cores device_configs display_remote enforce_cpuid floatformat_arm_ext_big floatformat_arm_ext_littlebyte_bigword floatformat_i387_ext floatformat_i960_ext floatformat_ia64_quad_big floatformat_ia64_quad_little floatformat_ia64_spill_big floatformat_ia64_spill_little floatformat_ieee_double_big floatformat_ieee_double_little floatformat_ieee_double_littlebyte_bigword floatformat_ieee_single_big floatformat_ieee_single_little floatformat_m68881_ext floatformat_m88110_ext floatformat_m88110_harris_ext fsl_register_prefix fw_boot_order last_mapping_addr last_mapping_sym last_type leon3_generic_machine m68k_numaliases m68k_numopcodes m68k_opcode_aliases m68k_opcodes memory_region_transaction_depth mips_builtin_opcodes mips_fulong2e_machine mips_opcodes no_reboot num_powerpc_operands para_features powerpc_macros powerpc_num_macros powerpc_num_opcodes powerpc_operands pvr_register_prefix qemu_boot_opts qemu_global_mutex qemu_option_rom_opts register_prefix rtas_next s390_virtio_bus_info sh_table special_register_prefix test_image timers_state v9fs_synth_root virtcon_hds vmstate_bmdma_status vmstate_info_scsi_requests vmstate_rxtx_stats xen_platform_ioport xen_xcg What about functions? ;-) I planned to check them separately, as I do in gnulib's sc_tight_scope syntax-check rule: http://git.sv.gnu.org/cgit/gnulib.git/tree/top/maint.mk#n1443 Interesting. We could do similar source level checks for patches in checkpatch.pl (which should be a commit hook if it never generated any false positives) and add the object file tests for example to the 'make check' test setup.
Re: [Qemu-devel] [PATCH 0/3] tighten scope of accidentally global variables
Blue Swirl wrote: On Mon, May 21, 2012 at 6:10 PM, Jim Meyering j...@meyering.net wrote: Blue Swirl wrote: On Mon, May 21, 2012 at 10:03 AM, Jim Meyering j...@meyering.net wrote: From: Jim Meyering meyer...@redhat.com I noticed this commit, virtio-pci: add missing 'static' which made this change: -const MemoryRegionPortio virtio_portio[] = { +static const MemoryRegionPortio virtio_portio[] = { and wondered if there were other variables like that. The following command shows that there are: [note that there are probably more: this finds only those for which the variable name appears in only one source file. ] Also, only for files at the top level. Thanks. There were so many .o files, I assumed that all were at the top. Searching all .o files, I found many more: $ for i in $(nm -e $(find . -name '*.o')|sed -n 's/.* [BCDGRS] //p'|sort -u);\ do test $(git grep -lw $i|wc -l) = 1 echo $i;done How about just checking the executables (*-softmmu/qemu-system-xyz and linux-user/qemu-xyz)? If I understood what you suggest, that would seem to miss only test_image: (README-scope-vars is the result of the one above using find and *.o) $ for i in $(nm -e *-softmmu/qemu-system-* linux-user/qemu-*|sed -n 's/.* [BCDGRS] //p'|sort -u); do test $(git grep -lw $i|wc -l) = 1 echo $i;done k nm: linux-user/qemu-types.h: File format not recognized [Exit 1] $ diff -u k README-scope-vars --- k 2012-05-21 20:38:15.912149444 +0200 +++ README-scope-vars 2012-05-21 20:12:13.270023293 +0200 @@ -53,6 +62,7 @@ rtas_next s390_virtio_bus_info sh_table special_register_prefix +test_image timers_state v9fs_synth_root virtcon_hds