[Qemu-devel] [PATCH 0/3] tighten scope of accidentally global variables

[Qemu-devel] [PATCH 0/3] tighten scope of accidentally global variables

[Jim Meyering]

From: Jim Meyering <meyering@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

[Qemu-devel] [PATCH 1/3] xen: remove unused global, xen_xcg

[Jim Meyering]

From: Jim Meyering <meyering@redhat.com>


Signed-off-by: Jim Meyering <meyering@redhat.com>
---
 hw/xen_backend.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/hw/xen_backend.c b/hw/xen_backend.c
index 66cb144..e44ced0 100644
--- a/hw/xen_backend.c
+++ b/hw/xen_backend.c
@@ -47,7 +47,6 @@

 /* public */
 XenXC xen_xc = XC_HANDLER_INITIAL_VALUE;
-XenGnttab xen_xcg = XC_HANDLER_INITIAL_VALUE;
 struct xs_handle *xenstore = NULL;
 const char *xen_protocol;

-- 
1.7.10.2.552.gaa3bb87

Re: [Qemu-devel] [PATCH 1/3] xen: remove unused global, xen_xcg

[Stefano Stabellini]

On Mon, 21 May 2012, Jim Meyering wrote:
> From: Jim Meyering <meyering@redhat.com>
> 
> 
> Signed-off-by: Jim Meyering <meyering@redhat.com>

Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>


>  hw/xen_backend.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/hw/xen_backend.c b/hw/xen_backend.c
> index 66cb144..e44ced0 100644
> --- a/hw/xen_backend.c
> +++ b/hw/xen_backend.c
> @@ -47,7 +47,6 @@
> 
>  /* public */
>  XenXC xen_xc = XC_HANDLER_INITIAL_VALUE;
> -XenGnttab xen_xcg = XC_HANDLER_INITIAL_VALUE;
>  struct xs_handle *xenstore = NULL;
>  const char *xen_protocol;
> 
> -- 
> 1.7.10.2.552.gaa3bb87
> 

[Qemu-devel] [PATCH 2/3] scsi: declare vmstate_info_scsi_requests to be static

[Jim Meyering]

From: Jim Meyering <meyering@redhat.com>


Signed-off-by: Jim Meyering <meyering@redhat.com>
---
 hw/scsi-bus.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/scsi-bus.c b/hw/scsi-bus.c
index 8ab9bcd..f10f3ec 100644
--- a/hw/scsi-bus.c
+++ b/hw/scsi-bus.c
@@ -1561,7 +1561,7 @@ static int get_scsi_requests(QEMUFile *f, void *pv, size_t size)
     return 0;
 }

-const VMStateInfo vmstate_info_scsi_requests = {
+static const VMStateInfo vmstate_info_scsi_requests = {
     .name = "scsi-requests",
     .get  = get_scsi_requests,
     .put  = put_scsi_requests,
-- 
1.7.10.2.552.gaa3bb87

[Qemu-devel] [PATCH 3/3] qemu-config: qemu_option_rom_opts, qemu_boot_opts: declare static

[Jim Meyering]

From: Jim Meyering <meyering@redhat.com>


Signed-off-by: Jim Meyering <meyering@redhat.com>
---
 qemu-config.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/qemu-config.c b/qemu-config.c
index be84a03..c03e52b 100644
--- a/qemu-config.c
+++ b/qemu-config.c
@@ -528,7 +528,7 @@ QemuOptsList qemu_spice_opts = {
     },
 };

-QemuOptsList qemu_option_rom_opts = {
+static QemuOptsList qemu_option_rom_opts = {
     .name = "option-rom",
     .implied_opt_name = "romfile",
     .head = QTAILQ_HEAD_INITIALIZER(qemu_option_rom_opts.head),
@@ -587,7 +587,7 @@ static QemuOptsList qemu_machine_opts = {
     },
 };

-QemuOptsList qemu_boot_opts = {
+static QemuOptsList qemu_boot_opts = {
     .name = "boot-opts",
     .head = QTAILQ_HEAD_INITIALIZER(qemu_boot_opts.head),
     .desc = {
-- 
1.7.10.2.552.gaa3bb87

Re: [Qemu-devel] [PATCH 0/3] tighten scope of accidentally global variables

[Blue Swirl]

On Mon, May 21, 2012 at 10:03 AM, Jim Meyering <jim@meyering.net> wrote:
> From: Jim Meyering <meyering@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

[Jim Meyering]

Blue Swirl wrote:
> On Mon, May 21, 2012 at 10:03 AM, Jim Meyering <jim@meyering.net> wrote:
>> From: Jim Meyering <meyering@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

[Blue Swirl]

On Mon, May 21, 2012 at 6:10 PM, Jim Meyering <jim@meyering.net> wrote:
> Blue Swirl wrote:
>> On Mon, May 21, 2012 at 10:03 AM, Jim Meyering <jim@meyering.net> wrote:
>>> From: Jim Meyering <meyering@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

[Jim Meyering]

Blue Swirl wrote:
> On Mon, May 21, 2012 at 6:10 PM, Jim Meyering <jim@meyering.net> wrote:
>> Blue Swirl wrote:
>>> On Mon, May 21, 2012 at 10:03 AM, Jim Meyering <jim@meyering.net> wrote:
>>>> From: Jim Meyering <meyering@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