Rename variables to avoid shadowing and thus address
MISRA C:2012 Rule 5.3:
"An identifier declared in an inner scope shall not hide an
identifier declared in an outer scope"
No functional changes.
Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
---
xen/arch/x86/hvm/mtrr.c | 32 ++++++++++++++++----------------
1 file changed, 16 insertions(+), 16 deletions(-)
diff --git a/xen/arch/x86/hvm/mtrr.c b/xen/arch/x86/hvm/mtrr.c
index 29f3fb1607..d504d1e43b 100644
--- a/xen/arch/x86/hvm/mtrr.c
+++ b/xen/arch/x86/hvm/mtrr.c
@@ -687,13 +687,13 @@ int hvm_set_mem_pinned_cacheattr(struct domain *d, uint64_t gfn_start,
static int cf_check hvm_save_mtrr_msr(struct vcpu *v, hvm_domain_context_t *h)
{
- const struct mtrr_state *mtrr_state = &v->arch.hvm.mtrr;
+ const struct mtrr_state *mtrr = &v->arch.hvm.mtrr;
struct hvm_hw_mtrr hw_mtrr = {
- .msr_mtrr_def_type = mtrr_state->def_type |
- MASK_INSR(mtrr_state->fixed_enabled,
+ .msr_mtrr_def_type = mtrr->def_type |
+ MASK_INSR(mtrr->fixed_enabled,
MTRRdefType_FE) |
- MASK_INSR(mtrr_state->enabled, MTRRdefType_E),
- .msr_mtrr_cap = mtrr_state->mtrr_cap,
+ MASK_INSR(mtrr->enabled, MTRRdefType_E),
+ .msr_mtrr_cap = mtrr->mtrr_cap,
};
unsigned int i;
@@ -710,14 +710,14 @@ static int cf_check hvm_save_mtrr_msr(struct vcpu *v, hvm_domain_context_t *h)
for ( i = 0; i < MASK_EXTR(hw_mtrr.msr_mtrr_cap, MTRRcap_VCNT); i++ )
{
- hw_mtrr.msr_mtrr_var[i * 2] = mtrr_state->var_ranges->base;
- hw_mtrr.msr_mtrr_var[i * 2 + 1] = mtrr_state->var_ranges->mask;
+ hw_mtrr.msr_mtrr_var[i * 2] = mtrr->var_ranges->base;
+ hw_mtrr.msr_mtrr_var[i * 2 + 1] = mtrr->var_ranges->mask;
}
BUILD_BUG_ON(sizeof(hw_mtrr.msr_mtrr_fixed) !=
- sizeof(mtrr_state->fixed_ranges));
+ sizeof(mtrr->fixed_ranges));
- memcpy(hw_mtrr.msr_mtrr_fixed, mtrr_state->fixed_ranges,
+ memcpy(hw_mtrr.msr_mtrr_fixed, mtrr->fixed_ranges,
sizeof(hw_mtrr.msr_mtrr_fixed));
return hvm_save_entry(MTRR, v->vcpu_id, h, &hw_mtrr);
@@ -727,7 +727,7 @@ static int cf_check hvm_load_mtrr_msr(struct domain *d, hvm_domain_context_t *h)
{
unsigned int vcpuid, i;
struct vcpu *v;
- struct mtrr_state *mtrr_state;
+ struct mtrr_state *mtrr;
struct hvm_hw_mtrr hw_mtrr;
vcpuid = hvm_load_instance(h);
@@ -749,26 +749,26 @@ static int cf_check hvm_load_mtrr_msr(struct domain *d, hvm_domain_context_t *h)
return -EINVAL;
}
- mtrr_state = &v->arch.hvm.mtrr;
+ mtrr = &v->arch.hvm.mtrr;
hvm_set_guest_pat(v, hw_mtrr.msr_pat_cr);
- mtrr_state->mtrr_cap = hw_mtrr.msr_mtrr_cap;
+ mtrr->mtrr_cap = hw_mtrr.msr_mtrr_cap;
for ( i = 0; i < NUM_FIXED_MSR; i++ )
- mtrr_fix_range_msr_set(d, mtrr_state, i, hw_mtrr.msr_mtrr_fixed[i]);
+ mtrr_fix_range_msr_set(d, mtrr, i, hw_mtrr.msr_mtrr_fixed[i]);
for ( i = 0; i < MASK_EXTR(hw_mtrr.msr_mtrr_cap, MTRRcap_VCNT); i++ )
{
- mtrr_var_range_msr_set(d, mtrr_state,
+ mtrr_var_range_msr_set(d, mtrr,
MSR_IA32_MTRR_PHYSBASE(i),
hw_mtrr.msr_mtrr_var[i * 2]);
- mtrr_var_range_msr_set(d, mtrr_state,
+ mtrr_var_range_msr_set(d, mtrr,
MSR_IA32_MTRR_PHYSMASK(i),
hw_mtrr.msr_mtrr_var[i * 2 + 1]);
}
- mtrr_def_type_msr_set(d, mtrr_state, hw_mtrr.msr_mtrr_def_type);
+ mtrr_def_type_msr_set(d, mtrr, hw_mtrr.msr_mtrr_def_type);
return 0;
}
--
2.34.1On Wed, 2 Aug 2023, Nicola Vetrini wrote:
> Rename variables to avoid shadowing and thus address
> MISRA C:2012 Rule 5.3:
> "An identifier declared in an inner scope shall not hide an
> identifier declared in an outer scope"
>
> No functional changes.
>
> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
This one clashes with xen/arch/x86/include/asm/mtrr.h:struct mtrr_state
right?
> ---
> xen/arch/x86/hvm/mtrr.c | 32 ++++++++++++++++----------------
> 1 file changed, 16 insertions(+), 16 deletions(-)
>
> diff --git a/xen/arch/x86/hvm/mtrr.c b/xen/arch/x86/hvm/mtrr.c
> index 29f3fb1607..d504d1e43b 100644
> --- a/xen/arch/x86/hvm/mtrr.c
> +++ b/xen/arch/x86/hvm/mtrr.c
> @@ -687,13 +687,13 @@ int hvm_set_mem_pinned_cacheattr(struct domain *d, uint64_t gfn_start,
>
> static int cf_check hvm_save_mtrr_msr(struct vcpu *v, hvm_domain_context_t *h)
> {
> - const struct mtrr_state *mtrr_state = &v->arch.hvm.mtrr;
> + const struct mtrr_state *mtrr = &v->arch.hvm.mtrr;
I can see in hvm_vcpu_cacheattr_init, mtrr_get_type and other places
that we use:
const struct mtrr_state *m
maybe that's better? I'll let Jan comment.
Anyway, the patch looks correct to me.
> struct hvm_hw_mtrr hw_mtrr = {
> - .msr_mtrr_def_type = mtrr_state->def_type |
> - MASK_INSR(mtrr_state->fixed_enabled,
> + .msr_mtrr_def_type = mtrr->def_type |
> + MASK_INSR(mtrr->fixed_enabled,
> MTRRdefType_FE) |
> - MASK_INSR(mtrr_state->enabled, MTRRdefType_E),
> - .msr_mtrr_cap = mtrr_state->mtrr_cap,
> + MASK_INSR(mtrr->enabled, MTRRdefType_E),
> + .msr_mtrr_cap = mtrr->mtrr_cap,
> };
> unsigned int i;
>
> @@ -710,14 +710,14 @@ static int cf_check hvm_save_mtrr_msr(struct vcpu *v, hvm_domain_context_t *h)
>
> for ( i = 0; i < MASK_EXTR(hw_mtrr.msr_mtrr_cap, MTRRcap_VCNT); i++ )
> {
> - hw_mtrr.msr_mtrr_var[i * 2] = mtrr_state->var_ranges->base;
> - hw_mtrr.msr_mtrr_var[i * 2 + 1] = mtrr_state->var_ranges->mask;
> + hw_mtrr.msr_mtrr_var[i * 2] = mtrr->var_ranges->base;
> + hw_mtrr.msr_mtrr_var[i * 2 + 1] = mtrr->var_ranges->mask;
> }
>
> BUILD_BUG_ON(sizeof(hw_mtrr.msr_mtrr_fixed) !=
> - sizeof(mtrr_state->fixed_ranges));
> + sizeof(mtrr->fixed_ranges));
>
> - memcpy(hw_mtrr.msr_mtrr_fixed, mtrr_state->fixed_ranges,
> + memcpy(hw_mtrr.msr_mtrr_fixed, mtrr->fixed_ranges,
> sizeof(hw_mtrr.msr_mtrr_fixed));
>
> return hvm_save_entry(MTRR, v->vcpu_id, h, &hw_mtrr);
> @@ -727,7 +727,7 @@ static int cf_check hvm_load_mtrr_msr(struct domain *d, hvm_domain_context_t *h)
> {
> unsigned int vcpuid, i;
> struct vcpu *v;
> - struct mtrr_state *mtrr_state;
> + struct mtrr_state *mtrr;
> struct hvm_hw_mtrr hw_mtrr;
>
> vcpuid = hvm_load_instance(h);
> @@ -749,26 +749,26 @@ static int cf_check hvm_load_mtrr_msr(struct domain *d, hvm_domain_context_t *h)
> return -EINVAL;
> }
>
> - mtrr_state = &v->arch.hvm.mtrr;
> + mtrr = &v->arch.hvm.mtrr;
>
> hvm_set_guest_pat(v, hw_mtrr.msr_pat_cr);
>
> - mtrr_state->mtrr_cap = hw_mtrr.msr_mtrr_cap;
> + mtrr->mtrr_cap = hw_mtrr.msr_mtrr_cap;
>
> for ( i = 0; i < NUM_FIXED_MSR; i++ )
> - mtrr_fix_range_msr_set(d, mtrr_state, i, hw_mtrr.msr_mtrr_fixed[i]);
> + mtrr_fix_range_msr_set(d, mtrr, i, hw_mtrr.msr_mtrr_fixed[i]);
>
> for ( i = 0; i < MASK_EXTR(hw_mtrr.msr_mtrr_cap, MTRRcap_VCNT); i++ )
> {
> - mtrr_var_range_msr_set(d, mtrr_state,
> + mtrr_var_range_msr_set(d, mtrr,
> MSR_IA32_MTRR_PHYSBASE(i),
> hw_mtrr.msr_mtrr_var[i * 2]);
> - mtrr_var_range_msr_set(d, mtrr_state,
> + mtrr_var_range_msr_set(d, mtrr,
> MSR_IA32_MTRR_PHYSMASK(i),
> hw_mtrr.msr_mtrr_var[i * 2 + 1]);
> }
>
> - mtrr_def_type_msr_set(d, mtrr_state, hw_mtrr.msr_mtrr_def_type);
> + mtrr_def_type_msr_set(d, mtrr, hw_mtrr.msr_mtrr_def_type);
>
> return 0;
> }
> --
> 2.34.1
>
On 03.08.2023 03:51, Stefano Stabellini wrote:
> On Wed, 2 Aug 2023, Nicola Vetrini wrote:
>> --- a/xen/arch/x86/hvm/mtrr.c
>> +++ b/xen/arch/x86/hvm/mtrr.c
>> @@ -687,13 +687,13 @@ int hvm_set_mem_pinned_cacheattr(struct domain *d, uint64_t gfn_start,
>>
>> static int cf_check hvm_save_mtrr_msr(struct vcpu *v, hvm_domain_context_t *h)
>> {
>> - const struct mtrr_state *mtrr_state = &v->arch.hvm.mtrr;
>> + const struct mtrr_state *mtrr = &v->arch.hvm.mtrr;
>
> I can see in hvm_vcpu_cacheattr_init, mtrr_get_type and other places
> that we use:
>
> const struct mtrr_state *m
>
> maybe that's better? I'll let Jan comment.
When unambiguous, personally I prefer shorter names (and as you say
existing code supports me in that). Obviously the longer a function,
the more potential future ambiguities also need taking into
consideration.
Jan
On 03/08/2023 03:51, Stefano Stabellini wrote:
> On Wed, 2 Aug 2023, Nicola Vetrini wrote:
>> Rename variables to avoid shadowing and thus address
>> MISRA C:2012 Rule 5.3:
>> "An identifier declared in an inner scope shall not hide an
>> identifier declared in an outer scope"
>>
>> No functional changes.
>>
>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
>
> This one clashes with xen/arch/x86/include/asm/mtrr.h:struct mtrr_state
> right?
>
Yes, indeed. You can also check this when in doubt at
https://saas.eclairit.com, but I do agree that mentioning it directly in
the commit
would make review easier.
>
>> ---
>> xen/arch/x86/hvm/mtrr.c | 32 ++++++++++++++++----------------
>> 1 file changed, 16 insertions(+), 16 deletions(-)
>>
>> diff --git a/xen/arch/x86/hvm/mtrr.c b/xen/arch/x86/hvm/mtrr.c
>> index 29f3fb1607..d504d1e43b 100644
>> --- a/xen/arch/x86/hvm/mtrr.c
>> +++ b/xen/arch/x86/hvm/mtrr.c
>> @@ -687,13 +687,13 @@ int hvm_set_mem_pinned_cacheattr(struct domain
>> *d, uint64_t gfn_start,
>>
>> static int cf_check hvm_save_mtrr_msr(struct vcpu *v,
>> hvm_domain_context_t *h)
>> {
>> - const struct mtrr_state *mtrr_state = &v->arch.hvm.mtrr;
>> + const struct mtrr_state *mtrr = &v->arch.hvm.mtrr;
>
> I can see in hvm_vcpu_cacheattr_init, mtrr_get_type and other places
> that we use:
>
> const struct mtrr_state *m
>
> maybe that's better? I'll let Jan comment.
>
> Anyway, the patch looks correct to me.
>
>
I though about it, and it seemed too generic, so in the end I decided
for mtrr, but
as always with names, I'm open to alternative suggestions.
--
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)
© 2016 - 2026 Red Hat, Inc.