diff mbox series

[XEN,1/4] x86: mechanically rename to address MISRA C:2012 Rule 5.3

Message ID 614dd6fe03ab82db83eabc430b27435b19096791.1690449587.git.nicola.vetrini@bugseng.com (mailing list archive)
State New, archived
Headers show
Series address violations of MISRA C:2012 Rule 5.3 on x86 | expand

Commit Message

Nicola Vetrini July 27, 2023, 10:47 a.m. UTC
Rule 5.3 has the following headline:
"An identifier declared in an inner scope shall not hide an
identifier declared in an outer scope"

The renames done by this patch avoid shadowing from happening.
They are as follows:
- s/str/s/ in 'lapic_disable'
- s/str/level/ in '(apic|mce)_set_verbosity'
- s/str/state_str/ in 'mwait_idle_probe'
- s/str/memmap_name/ in 'init_e820'
- s/i/j/ in 'mce_action' (the shadowing here is due to macro
  'x86_mcinfo_lookup' that defines 'i' as a loop counter)
- s/desc/descriptor/ in '_hvm_load_entry'
- s/socket_info/sock_info/ in 'do_write_psr_msrs'
- s/debug_stack_lines/dbg_stack_lines/ in 'compat_show_guest_stack'

The parameter 'cpu_khz' that causes a violation in 'pit_init'
is unused, and hence can be removed.

Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
---
Function 'str' in 'xen/arch/x86/include/asm/desc.h'
causes the shadowing.
---
 xen/arch/x86/apic.c                 |  8 ++++----
 xen/arch/x86/cpu/mcheck/mce.c       | 12 ++++++------
 xen/arch/x86/cpu/mwait-idle.c       | 24 ++++++++++++------------
 xen/arch/x86/domain.c               |  2 +-
 xen/arch/x86/e820.c                 |  6 +++---
 xen/arch/x86/emul-i8254.c           |  2 +-
 xen/arch/x86/include/asm/e820.h     |  2 +-
 xen/arch/x86/include/asm/hvm/save.h |  8 ++++----
 xen/arch/x86/include/asm/hvm/vpt.h  |  2 +-
 xen/arch/x86/psr.c                  |  4 ++--
 xen/arch/x86/traps.c                |  4 ++--
 11 files changed, 37 insertions(+), 37 deletions(-)

Comments

Jan Beulich July 27, 2023, 2:50 p.m. UTC | #1
On 27.07.2023 12:47, Nicola Vetrini wrote:
> Rule 5.3 has the following headline:
> "An identifier declared in an inner scope shall not hide an
> identifier declared in an outer scope"
> 
> The renames done by this patch avoid shadowing from happening.
> They are as follows:
> - s/str/s/ in 'lapic_disable'
> - s/str/level/ in '(apic|mce)_set_verbosity'
> - s/str/state_str/ in 'mwait_idle_probe'
> - s/str/memmap_name/ in 'init_e820'

I'm sorry to say that, but I'm not willing to go and figure out where
that "str" is that there's supposedly a collision with. Please can you
state such right here, ...

> - s/i/j/ in 'mce_action' (the shadowing here is due to macro
>   'x86_mcinfo_lookup' that defines 'i' as a loop counter)

... much like you do in this case?

> - s/desc/descriptor/ in '_hvm_load_entry'
> - s/socket_info/sock_info/ in 'do_write_psr_msrs'

(I didn't look at any of these in any detail, partly because again
I hope for additional context before doing so.)

> - s/debug_stack_lines/dbg_stack_lines/ in 'compat_show_guest_stack'

This wants doing differently: The two functions originally lived in
different source files, so passing the static variable as argument
was preferred over making the variable non-static. When the function
was moved, that aspect was overlooked. The function argument simply
wants dropping.

Jan
Andrew Cooper July 27, 2023, 3 p.m. UTC | #2
On 27/07/2023 3:50 pm, Jan Beulich wrote:
> On 27.07.2023 12:47, Nicola Vetrini wrote:
>> Rule 5.3 has the following headline:
>> "An identifier declared in an inner scope shall not hide an
>> identifier declared in an outer scope"
>>
>> The renames done by this patch avoid shadowing from happening.
>> They are as follows:
>> - s/str/s/ in 'lapic_disable'
>> - s/str/level/ in '(apic|mce)_set_verbosity'
>> - s/str/state_str/ in 'mwait_idle_probe'
>> - s/str/memmap_name/ in 'init_e820'
> I'm sorry to say that, but I'm not willing to go and figure out where
> that "str" is that there's supposedly a collision with. Please can you
> state such right here, ...
>
>> - s/i/j/ in 'mce_action' (the shadowing here is due to macro
>>   'x86_mcinfo_lookup' that defines 'i' as a loop counter)
> ... much like you do in this case?

In fairness to Nicola, that was given.

> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
> ---
> Function 'str' in 'xen/arch/x86/include/asm/desc.h'
> causes the shadowing.

which is the wrapper for the STR instruction.

It's used in a single assertion, and I'd be happy getting rid of it
entirely.  Alternatively, it could be renamed to read_tr() (or
read_tr_sel() ?) if we want to keep the assertion.

We're not renaming every other use of 'str' to mean string just for this...

~Andrew
Nicola Vetrini July 27, 2023, 3:18 p.m. UTC | #3
On 27/07/23 17:00, Andrew Cooper wrote:
> On 27/07/2023 3:50 pm, Jan Beulich wrote:
>> On 27.07.2023 12:47, Nicola Vetrini wrote:
>>> Rule 5.3 has the following headline:
>>> "An identifier declared in an inner scope shall not hide an
>>> identifier declared in an outer scope"
>>>
>>> The renames done by this patch avoid shadowing from happening.
>>> They are as follows:
>>> - s/str/s/ in 'lapic_disable'
>>> - s/str/level/ in '(apic|mce)_set_verbosity'
>>> - s/str/state_str/ in 'mwait_idle_probe'
>>> - s/str/memmap_name/ in 'init_e820'
>> I'm sorry to say that, but I'm not willing to go and figure out where
>> that "str" is that there's supposedly a collision with. Please can you
>> state such right here, ...
>>
>>> - s/i/j/ in 'mce_action' (the shadowing here is due to macro
>>>    'x86_mcinfo_lookup' that defines 'i' as a loop counter)
>> ... much like you do in this case?
> 
> In fairness to Nicola, that was given.
> 
>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
>> ---
>> Function 'str' in 'xen/arch/x86/include/asm/desc.h'
>> causes the shadowing.
> 
> which is the wrapper for the STR instruction.
> 
> It's used in a single assertion, and I'd be happy getting rid of it
> entirely.  Alternatively, it could be renamed to read_tr() (or
> read_tr_sel() ?) if we want to keep the assertion.
> 
> We're not renaming every other use of 'str' to mean string just for this...
> 
> ~Andrew

Seems reasonable to remove it, though there aren't that many instances 
of shadowing on 'str'.
diff mbox series

Patch

diff --git a/xen/arch/x86/apic.c b/xen/arch/x86/apic.c
index 41879230ec..57ec500408 100644
--- a/xen/arch/x86/apic.c
+++ b/xen/arch/x86/apic.c
@@ -760,7 +760,7 @@  int lapic_resume(void)
  * Original code written by Keir Fraser.
  */
 
-static int __init cf_check lapic_disable(const char *str)
+static int __init cf_check lapic_disable(const char *s)
 {
     enable_local_apic = -1;
     setup_clear_cpu_cap(X86_FEATURE_APIC);
@@ -769,11 +769,11 @@  static int __init cf_check lapic_disable(const char *str)
 custom_param("nolapic", lapic_disable);
 boolean_param("lapic", enable_local_apic);
 
-static int __init cf_check apic_set_verbosity(const char *str)
+static int __init cf_check apic_set_verbosity(const char *level)
 {
-    if (strcmp("debug", str) == 0)
+    if (strcmp("debug", level) == 0)
         apic_verbosity = APIC_DEBUG;
-    else if (strcmp("verbose", str) == 0)
+    else if (strcmp("verbose", level) == 0)
         apic_verbosity = APIC_VERBOSE;
     else
         return -EINVAL;
diff --git a/xen/arch/x86/cpu/mcheck/mce.c b/xen/arch/x86/cpu/mcheck/mce.c
index c2173cfc78..9d76a462a7 100644
--- a/xen/arch/x86/cpu/mcheck/mce.c
+++ b/xen/arch/x86/cpu/mcheck/mce.c
@@ -63,9 +63,9 @@  struct mca_banks *mca_allbanks;
 #endif
 
 int mce_verbosity;
-static int __init cf_check mce_set_verbosity(const char *str)
+static int __init cf_check mce_set_verbosity(const char *level)
 {
-    if ( strcmp("verbose", str) == 0 )
+    if ( strcmp("verbose", level) == 0 )
         mce_verbosity = MCE_VERBOSE;
     else
         return -EINVAL;
@@ -1746,7 +1746,7 @@  static enum mce_result mce_action(const struct cpu_user_regs *regs,
     struct mcinfo_common *mic = NULL;
     struct mca_binfo binfo;
     const struct mca_error_handler *handlers = mce_dhandlers;
-    unsigned int i, handler_num = mce_dhandler_num;
+    unsigned int j, handler_num = mce_dhandler_num;
 
     /* When in mce context, regs is valid */
     if ( regs )
@@ -1780,11 +1780,11 @@  static enum mce_result mce_action(const struct cpu_user_regs *regs,
         binfo.mib = (struct mcinfo_bank *)mic;
         binfo.bank = binfo.mib->mc_bank;
         bank_result = MCER_NOERROR;
-        for ( i = 0; i < handler_num; i++ )
+        for ( j = 0; j < handler_num; j++ )
         {
-            if ( handlers[i].owned_error(binfo.mib->mc_status) )
+            if ( handlers[j].owned_error(binfo.mib->mc_status) )
             {
-                handlers[i].recovery_handler(&binfo, &bank_result, regs);
+                handlers[j].recovery_handler(&binfo, &bank_result, regs);
                 if ( worst_result < bank_result )
                     worst_result = bank_result;
                 break;
diff --git a/xen/arch/x86/cpu/mwait-idle.c b/xen/arch/x86/cpu/mwait-idle.c
index ff5c808bc9..8abe14773d 100644
--- a/xen/arch/x86/cpu/mwait-idle.c
+++ b/xen/arch/x86/cpu/mwait-idle.c
@@ -1429,7 +1429,7 @@  static int __init mwait_idle_probe(void)
 {
 	unsigned int eax, ebx, ecx;
 	const struct x86_cpu_id *id = x86_match_cpu(intel_idle_ids);
-	const char *str;
+	const char *state_str;
 
 	if (!id) {
 		pr_debug(PREFIX "does not run on family %d model %d\n",
@@ -1471,10 +1471,10 @@  static int __init mwait_idle_probe(void)
 	pr_debug(PREFIX "lapic_timer_reliable_states %#x\n",
 		 lapic_timer_reliable_states);
 
-	str = preferred_states;
-	if (isdigit(str[0]))
-		preferred_states_mask = simple_strtoul(str, &str, 0);
-	else if (str[0])
+	state_str = preferred_states;
+	if (isdigit(state_str[0]))
+		preferred_states_mask = simple_strtoul(state_str, &state_str, 0);
+	else if (state_str[0])
 	{
 		const char *ss;
 
@@ -1482,13 +1482,13 @@  static int __init mwait_idle_probe(void)
 			const struct cpuidle_state *state = icpu->state_table;
 			unsigned int bit = 1;
 
-			ss = strchr(str, ',');
+			ss = strchr(state_str, ',');
 			if (!ss)
-				ss = strchr(str, '\0');
+				ss = strchr(state_str, '\0');
 
 			for (; state->name[0]; ++state) {
 				bit <<= 1;
-				if (!cmdline_strcmp(str, state->name)) {
+				if (!cmdline_strcmp(state_str, state->name)) {
 					preferred_states_mask |= bit;
 					break;
 				}
@@ -1496,13 +1496,13 @@  static int __init mwait_idle_probe(void)
 			if (!state->name[0])
 				break;
 
-			str = ss + 1;
+			state_str = ss + 1;
 		} while (*ss);
 
-		str -= str == ss + 1;
+		state_str -= state_str == ss + 1;
 	}
-	if (str[0])
-		printk("unrecognized \"preferred-cstates=%s\"\n", str);
+	if (state_str[0])
+		printk("unrecognized \"preferred-cstates=%s\"\n", state_str);
 
 	mwait_idle_state_table_update();
 
diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index 5f66c2ae33..fe86a7f853 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -858,7 +858,7 @@  int arch_domain_create(struct domain *d,
     }
 
     /* PV/PVH guests get an emulated PIT too for video BIOSes to use. */
-    pit_init(d, cpu_khz);
+    pit_init(d);
 
     /*
      * If the FPU does not save FCS/FDS then we can always
diff --git a/xen/arch/x86/e820.c b/xen/arch/x86/e820.c
index 0b89935510..5bff35c4cf 100644
--- a/xen/arch/x86/e820.c
+++ b/xen/arch/x86/e820.c
@@ -683,11 +683,11 @@  int __init reserve_e820_ram(struct e820map *e820, uint64_t s, uint64_t e)
     return e820_change_range_type(e820, s, e, E820_RAM, E820_RESERVED);
 }
 
-unsigned long __init init_e820(const char *str, struct e820map *raw)
+unsigned long __init init_e820(const char *memmap_name, struct e820map *raw)
 {
     if ( e820_verbose )
     {
-        printk("Initial %s RAM map:\n", str);
+        printk("Initial %s RAM map:\n", memmap_name);
         print_e820_memory_map(raw->map, raw->nr_map);
     }
 
@@ -696,7 +696,7 @@  unsigned long __init init_e820(const char *str, struct e820map *raw)
     if ( cpu_has_hypervisor )
         hypervisor_e820_fixup(&e820);
 
-    printk("%s RAM map:\n", str);
+    printk("%s RAM map:\n", memmap_name);
     print_e820_memory_map(e820.map, e820.nr_map);
 
     return find_max_pfn();
diff --git a/xen/arch/x86/emul-i8254.c b/xen/arch/x86/emul-i8254.c
index a81232fc55..41ec4a1ef1 100644
--- a/xen/arch/x86/emul-i8254.c
+++ b/xen/arch/x86/emul-i8254.c
@@ -495,7 +495,7 @@  void pit_reset(struct domain *d)
     spin_unlock(&pit->lock);
 }
 
-void pit_init(struct domain *d, unsigned long cpu_khz)
+void pit_init(struct domain *d)
 {
     PITState *pit = domain_vpit(d);
 
diff --git a/xen/arch/x86/include/asm/e820.h b/xen/arch/x86/include/asm/e820.h
index 92f5efa4f5..5cb1915a6e 100644
--- a/xen/arch/x86/include/asm/e820.h
+++ b/xen/arch/x86/include/asm/e820.h
@@ -31,7 +31,7 @@  extern int e820_change_range_type(
     uint32_t orig_type, uint32_t new_type);
 extern int e820_add_range(
     struct e820map *, uint64_t s, uint64_t e, uint32_t type);
-extern unsigned long init_e820(const char *, struct e820map *);
+extern unsigned long init_e820(const char *memmap_name, struct e820map *raw);
 extern void print_e820_memory_map(const struct e820entry *map,
     unsigned int entries);
 extern struct e820map e820;
diff --git a/xen/arch/x86/include/asm/hvm/save.h b/xen/arch/x86/include/asm/hvm/save.h
index 535cf68ed2..4d0a062b2c 100644
--- a/xen/arch/x86/include/asm/hvm/save.h
+++ b/xen/arch/x86/include/asm/hvm/save.h
@@ -49,21 +49,21 @@  void _hvm_read_entry(struct hvm_domain_context *h,
  */
 #define _hvm_load_entry(_x, _h, _dst, _strict) ({                       \
     int r;                                                              \
-    struct hvm_save_descriptor *desc                                    \
+    struct hvm_save_descriptor *descriptor                              \
         = (struct hvm_save_descriptor *)&(_h)->data[(_h)->cur];         \
     if ( (r = _hvm_check_entry((_h), HVM_SAVE_CODE(_x),                 \
                HVM_SAVE_LENGTH(_x), (_strict))) == 0 )                  \
     {                                                                   \
         _hvm_read_entry((_h), (_dst), HVM_SAVE_LENGTH(_x));             \
         if ( HVM_SAVE_HAS_COMPAT(_x) &&                                 \
-             desc->length != HVM_SAVE_LENGTH(_x) )                      \
-            r = HVM_SAVE_FIX_COMPAT(_x, (_dst), desc->length);          \
+             descriptor->length != HVM_SAVE_LENGTH(_x) )                \
+            r = HVM_SAVE_FIX_COMPAT(_x, (_dst), descriptor->length);    \
     }                                                                   \
     else if (HVM_SAVE_HAS_COMPAT(_x)                                    \
              && (r = _hvm_check_entry((_h), HVM_SAVE_CODE(_x),          \
                        HVM_SAVE_LENGTH_COMPAT(_x), (_strict))) == 0 ) { \
         _hvm_read_entry((_h), (_dst), HVM_SAVE_LENGTH_COMPAT(_x));      \
-        r = HVM_SAVE_FIX_COMPAT(_x, (_dst), desc->length);              \
+        r = HVM_SAVE_FIX_COMPAT(_x, (_dst), descriptor->length);        \
     }                                                                   \
     r; })
 
diff --git a/xen/arch/x86/include/asm/hvm/vpt.h b/xen/arch/x86/include/asm/hvm/vpt.h
index 2af76ca8dc..feb0bf43f1 100644
--- a/xen/arch/x86/include/asm/hvm/vpt.h
+++ b/xen/arch/x86/include/asm/hvm/vpt.h
@@ -179,7 +179,7 @@  void destroy_periodic_time(struct periodic_time *pt);
 int pv_pit_handler(int port, int data, int write);
 void pit_reset(struct domain *d);
 
-void pit_init(struct domain *d, unsigned long cpu_khz);
+void pit_init(struct domain *d);
 void pit_stop_channel0_irq(PITState * pit);
 void pit_deinit(struct domain *d);
 void rtc_init(struct domain *d);
diff --git a/xen/arch/x86/psr.c b/xen/arch/x86/psr.c
index a1e0af27c5..90bcb25e60 100644
--- a/xen/arch/x86/psr.c
+++ b/xen/arch/x86/psr.c
@@ -1252,7 +1252,7 @@  static void cf_check do_write_psr_msrs(void *data)
 {
     const struct cos_write_info *info = data;
     unsigned int i, index, cos = info->cos;
-    const struct psr_socket_info *socket_info =
+    const struct psr_socket_info *sock_info =
         get_socket_info(cpu_to_socket(smp_processor_id()));
 
     /*
@@ -1261,7 +1261,7 @@  static void cf_check do_write_psr_msrs(void *data)
      */
     for ( index = i = 0; i < ARRAY_SIZE(feat_props); i++ )
     {
-        struct feat_node *feat = socket_info->features[i];
+        struct feat_node *feat = sock_info->features[i];
         const struct feat_props *props = feat_props[i];
         unsigned int cos_num, j;
 
diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index 4229bda159..f32cd2126d 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -196,7 +196,7 @@  void show_code(const struct cpu_user_regs *regs)
 
 static void compat_show_guest_stack(struct vcpu *v,
                                     const struct cpu_user_regs *regs,
-                                    int debug_stack_lines)
+                                    int dbg_stack_lines)
 {
     unsigned int i, *stack, addr, mask = STACK_SIZE;
     void *stack_page = NULL;
@@ -233,7 +233,7 @@  static void compat_show_guest_stack(struct vcpu *v,
             mask = PAGE_SIZE;
     }
 
-    for ( i = 0; i < debug_stack_lines * 8; i++ )
+    for ( i = 0; i < dbg_stack_lines * 8; i++ )
     {
         if ( (((long)stack - 1) ^ ((long)(stack + 1) - 1)) & mask )
             break;