Rule 5.3 has the following headline:
"An identifier declared in an inner scope shall not hide an
identifier declared in an outer scope"
Local variables have been suitably renamed to address some violations
of this rule:
- s/cmp/c/ because it shadows the union declared at line 87.
- s/nodes/numa_nodes/ shadows the static variable declared at line 18.
- s/ctrl/controller/ because the homonymous function parameter is later
read.
- s/baud/baud_rate/ to avoid shadowing the enum constant defined
at line 1391.
No functional change.
Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
---
xen/common/compat/memory.c | 6 +++---
xen/common/numa.c | 36 ++++++++++++++++++------------------
xen/drivers/char/ehci-dbgp.c | 4 ++--
xen/drivers/char/ns16550.c | 4 ++--
4 files changed, 25 insertions(+), 25 deletions(-)
diff --git a/xen/common/compat/memory.c b/xen/common/compat/memory.c
index 8ca63ceda6..3cf0a37d00 100644
--- a/xen/common/compat/memory.c
+++ b/xen/common/compat/memory.c
@@ -321,12 +321,12 @@ int compat_memory_op(unsigned int cmd, XEN_GUEST_HANDLE_PARAM(void) compat)
case XENMEM_remove_from_physmap:
{
- struct compat_remove_from_physmap cmp;
+ struct compat_remove_from_physmap c;
- if ( copy_from_guest(&cmp, compat, 1) )
+ if ( copy_from_guest(&c, compat, 1) )
return -EFAULT;
- XLAT_remove_from_physmap(nat.xrfp, &cmp);
+ XLAT_remove_from_physmap(nat.xrfp, &c);
break;
}
diff --git a/xen/common/numa.c b/xen/common/numa.c
index fc1f7f665b..301f2cf374 100644
--- a/xen/common/numa.c
+++ b/xen/common/numa.c
@@ -382,7 +382,7 @@ static bool __init numa_process_nodes(paddr_t start, paddr_t end)
* 0 if memnodmap[] too small (or shift too small)
* -1 if node overlap or lost ram (shift too big)
*/
-static int __init populate_memnodemap(const struct node *nodes,
+static int __init populate_memnodemap(const struct node *numa_nodes,
unsigned int numnodes, unsigned int shift,
const nodeid_t *nodeids)
{
@@ -393,8 +393,8 @@ static int __init populate_memnodemap(const struct node *nodes,
for ( i = 0; i < numnodes; i++ )
{
- unsigned long spdx = paddr_to_pdx(nodes[i].start);
- unsigned long epdx = paddr_to_pdx(nodes[i].end - 1);
+ unsigned long spdx = paddr_to_pdx(numa_nodes[i].start);
+ unsigned long epdx = paddr_to_pdx(numa_nodes[i].end - 1);
if ( spdx > epdx )
continue;
@@ -440,7 +440,7 @@ static int __init allocate_cachealigned_memnodemap(void)
* The LSB of all start addresses in the node map is the value of the
* maximum possible shift.
*/
-static unsigned int __init extract_lsb_from_nodes(const struct node *nodes,
+static unsigned int __init extract_lsb_from_nodes(const struct node *numa_nodes,
nodeid_t numnodes,
const nodeid_t *nodeids)
{
@@ -449,8 +449,8 @@ static unsigned int __init extract_lsb_from_nodes(const struct node *nodes,
for ( i = 0; i < numnodes; i++ )
{
- unsigned long spdx = paddr_to_pdx(nodes[i].start);
- unsigned long epdx = paddr_to_pdx(nodes[i].end - 1) + 1;
+ unsigned long spdx = paddr_to_pdx(numa_nodes[i].start);
+ unsigned long epdx = paddr_to_pdx(numa_nodes[i].end - 1) + 1;
if ( spdx >= epdx )
continue;
@@ -475,10 +475,10 @@ static unsigned int __init extract_lsb_from_nodes(const struct node *nodes,
return i;
}
-int __init compute_hash_shift(const struct node *nodes,
+int __init compute_hash_shift(const struct node *numa_nodes,
unsigned int numnodes, const nodeid_t *nodeids)
{
- unsigned int shift = extract_lsb_from_nodes(nodes, numnodes, nodeids);
+ unsigned int shift = extract_lsb_from_nodes(numa_nodes, numnodes, nodeids);
if ( memnodemapsize <= ARRAY_SIZE(_memnodemap) )
memnodemap = _memnodemap;
@@ -487,7 +487,7 @@ int __init compute_hash_shift(const struct node *nodes,
printk(KERN_DEBUG "NUMA: Using %u for the hash shift\n", shift);
- if ( populate_memnodemap(nodes, numnodes, shift, nodeids) != 1 )
+ if ( populate_memnodemap(numa_nodes, numnodes, shift, nodeids) != 1 )
{
printk(KERN_INFO "Your memory is not aligned you need to "
"rebuild your hypervisor with a bigger NODEMAPSIZE "
@@ -541,7 +541,7 @@ static int __init numa_emulation(unsigned long start_pfn,
{
int ret;
unsigned int i;
- struct node nodes[MAX_NUMNODES];
+ struct node numa_nodes[MAX_NUMNODES];
uint64_t sz = pfn_to_paddr(end_pfn - start_pfn) / numa_fake;
/* Kludge needed for the hash function */
@@ -556,22 +556,22 @@ static int __init numa_emulation(unsigned long start_pfn,
sz = x;
}
- memset(&nodes, 0, sizeof(nodes));
+ memset(&numa_nodes, 0, sizeof(numa_nodes));
for ( i = 0; i < numa_fake; i++ )
{
- nodes[i].start = pfn_to_paddr(start_pfn) + i * sz;
+ numa_nodes[i].start = pfn_to_paddr(start_pfn) + i * sz;
if ( i == numa_fake - 1 )
- sz = pfn_to_paddr(end_pfn) - nodes[i].start;
+ sz = pfn_to_paddr(end_pfn) - numa_nodes[i].start;
- nodes[i].end = nodes[i].start + sz;
+ numa_nodes[i].end = numa_nodes[i].start + sz;
printk(KERN_INFO "Faking node %u at %"PRIx64"-%"PRIx64" (%"PRIu64"MB)\n",
- i, nodes[i].start, nodes[i].end,
- (nodes[i].end - nodes[i].start) >> 20);
+ i, numa_nodes[i].start, numa_nodes[i].end,
+ (numa_nodes[i].end - numa_nodes[i].start) >> 20);
node_set_online(i);
}
- ret = compute_hash_shift(nodes, numa_fake, NULL);
+ ret = compute_hash_shift(numa_nodes, numa_fake, NULL);
if ( ret < 0 )
{
printk(KERN_ERR "No NUMA hash function found. Emulation disabled.\n");
@@ -580,7 +580,7 @@ static int __init numa_emulation(unsigned long start_pfn,
memnode_shift = ret;
for_each_online_node ( i )
- setup_node_bootmem(i, nodes[i].start, nodes[i].end);
+ setup_node_bootmem(i, numa_nodes[i].start, numa_nodes[i].end);
numa_init_array();
diff --git a/xen/drivers/char/ehci-dbgp.c b/xen/drivers/char/ehci-dbgp.c
index 4d8d765122..22650663dc 100644
--- a/xen/drivers/char/ehci-dbgp.c
+++ b/xen/drivers/char/ehci-dbgp.c
@@ -424,9 +424,9 @@ static void dbgp_issue_command(struct ehci_dbgp *dbgp, u32 ctrl,
* checks to see if ACPI or some other initialization also
* reset the EHCI debug port.
*/
- u32 ctrl = readl(&dbgp->ehci_debug->control);
+ u32 controller = readl(&dbgp->ehci_debug->control);
- if ( ctrl & DBGP_ENABLED )
+ if ( controller & DBGP_ENABLED )
{
cmd |= CMD_RUN;
writel(cmd, &dbgp->ehci_regs->command);
diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
index 212a9c49ae..61c7a19c4d 100644
--- a/xen/drivers/char/ns16550.c
+++ b/xen/drivers/char/ns16550.c
@@ -1473,7 +1473,7 @@ static enum __init serial_param_type get_token(char *token, char **value)
static bool __init parse_positional(struct ns16550 *uart, char **str)
{
- int baud;
+ int baud_rate;
const char *conf;
char *name_val_pos;
@@ -1504,7 +1504,7 @@ static bool __init parse_positional(struct ns16550 *uart, char **str)
uart->baud = BAUD_AUTO;
conf += 4;
}
- else if ( (baud = simple_strtoul(conf, &conf, 10)) != 0 )
+ else if ( (baud_rate = simple_strtoul(conf, &conf, 10)) != 0 )
uart->baud = baud;
if ( *conf == '/' )
--
2.34.1On 31.07.2023 15:35, 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"
>
> Local variables have been suitably renamed to address some violations
> of this rule:
> - s/cmp/c/ because it shadows the union declared at line 87.
> - s/nodes/numa_nodes/ shadows the static variable declared at line 18.
> - s/ctrl/controller/ because the homonymous function parameter is later
> read.
> - s/baud/baud_rate/ to avoid shadowing the enum constant defined
> at line 1391.
>
> No functional change.
>
> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
> ---
> xen/common/compat/memory.c | 6 +++---
> xen/common/numa.c | 36 ++++++++++++++++++------------------
> xen/drivers/char/ehci-dbgp.c | 4 ++--
> xen/drivers/char/ns16550.c | 4 ++--
> 4 files changed, 25 insertions(+), 25 deletions(-)
This is an odd mix of files touched in a single patch. How about splitting
into two, one for common/ and one for drivers/?
> --- a/xen/common/compat/memory.c
> +++ b/xen/common/compat/memory.c
> @@ -321,12 +321,12 @@ int compat_memory_op(unsigned int cmd, XEN_GUEST_HANDLE_PARAM(void) compat)
>
> case XENMEM_remove_from_physmap:
> {
> - struct compat_remove_from_physmap cmp;
> + struct compat_remove_from_physmap c;
The intention of the outer scope cmp is to avoid such inner scope
ones then consuming extra stack space. This wants making part of the
union there.
> --- a/xen/common/numa.c
> +++ b/xen/common/numa.c
> @@ -382,7 +382,7 @@ static bool __init numa_process_nodes(paddr_t start, paddr_t end)
> * 0 if memnodmap[] too small (or shift too small)
> * -1 if node overlap or lost ram (shift too big)
> */
> -static int __init populate_memnodemap(const struct node *nodes,
> +static int __init populate_memnodemap(const struct node *numa_nodes,
> unsigned int numnodes, unsigned int shift,
> const nodeid_t *nodeids)
> {
> @@ -393,8 +393,8 @@ static int __init populate_memnodemap(const struct node *nodes,
>
> for ( i = 0; i < numnodes; i++ )
> {
> - unsigned long spdx = paddr_to_pdx(nodes[i].start);
> - unsigned long epdx = paddr_to_pdx(nodes[i].end - 1);
> + unsigned long spdx = paddr_to_pdx(numa_nodes[i].start);
> + unsigned long epdx = paddr_to_pdx(numa_nodes[i].end - 1);
>
> if ( spdx > epdx )
> continue;
> @@ -440,7 +440,7 @@ static int __init allocate_cachealigned_memnodemap(void)
> * The LSB of all start addresses in the node map is the value of the
> * maximum possible shift.
> */
> -static unsigned int __init extract_lsb_from_nodes(const struct node *nodes,
> +static unsigned int __init extract_lsb_from_nodes(const struct node *numa_nodes,
> nodeid_t numnodes,
> const nodeid_t *nodeids)
> {
> @@ -449,8 +449,8 @@ static unsigned int __init extract_lsb_from_nodes(const struct node *nodes,
>
> for ( i = 0; i < numnodes; i++ )
> {
> - unsigned long spdx = paddr_to_pdx(nodes[i].start);
> - unsigned long epdx = paddr_to_pdx(nodes[i].end - 1) + 1;
> + unsigned long spdx = paddr_to_pdx(numa_nodes[i].start);
> + unsigned long epdx = paddr_to_pdx(numa_nodes[i].end - 1) + 1;
>
> if ( spdx >= epdx )
> continue;
> @@ -475,10 +475,10 @@ static unsigned int __init extract_lsb_from_nodes(const struct node *nodes,
> return i;
> }
>
> -int __init compute_hash_shift(const struct node *nodes,
> +int __init compute_hash_shift(const struct node *numa_nodes,
> unsigned int numnodes, const nodeid_t *nodeids)
> {
> - unsigned int shift = extract_lsb_from_nodes(nodes, numnodes, nodeids);
> + unsigned int shift = extract_lsb_from_nodes(numa_nodes, numnodes, nodeids);
>
> if ( memnodemapsize <= ARRAY_SIZE(_memnodemap) )
> memnodemap = _memnodemap;
> @@ -487,7 +487,7 @@ int __init compute_hash_shift(const struct node *nodes,
>
> printk(KERN_DEBUG "NUMA: Using %u for the hash shift\n", shift);
>
> - if ( populate_memnodemap(nodes, numnodes, shift, nodeids) != 1 )
> + if ( populate_memnodemap(numa_nodes, numnodes, shift, nodeids) != 1 )
> {
> printk(KERN_INFO "Your memory is not aligned you need to "
> "rebuild your hypervisor with a bigger NODEMAPSIZE "
> @@ -541,7 +541,7 @@ static int __init numa_emulation(unsigned long start_pfn,
> {
> int ret;
> unsigned int i;
> - struct node nodes[MAX_NUMNODES];
> + struct node numa_nodes[MAX_NUMNODES];
> uint64_t sz = pfn_to_paddr(end_pfn - start_pfn) / numa_fake;
>
> /* Kludge needed for the hash function */
> @@ -556,22 +556,22 @@ static int __init numa_emulation(unsigned long start_pfn,
> sz = x;
> }
>
> - memset(&nodes, 0, sizeof(nodes));
> + memset(&numa_nodes, 0, sizeof(numa_nodes));
> for ( i = 0; i < numa_fake; i++ )
> {
> - nodes[i].start = pfn_to_paddr(start_pfn) + i * sz;
> + numa_nodes[i].start = pfn_to_paddr(start_pfn) + i * sz;
>
> if ( i == numa_fake - 1 )
> - sz = pfn_to_paddr(end_pfn) - nodes[i].start;
> + sz = pfn_to_paddr(end_pfn) - numa_nodes[i].start;
>
> - nodes[i].end = nodes[i].start + sz;
> + numa_nodes[i].end = numa_nodes[i].start + sz;
> printk(KERN_INFO "Faking node %u at %"PRIx64"-%"PRIx64" (%"PRIu64"MB)\n",
> - i, nodes[i].start, nodes[i].end,
> - (nodes[i].end - nodes[i].start) >> 20);
> + i, numa_nodes[i].start, numa_nodes[i].end,
> + (numa_nodes[i].end - numa_nodes[i].start) >> 20);
> node_set_online(i);
> }
>
> - ret = compute_hash_shift(nodes, numa_fake, NULL);
> + ret = compute_hash_shift(numa_nodes, numa_fake, NULL);
> if ( ret < 0 )
> {
> printk(KERN_ERR "No NUMA hash function found. Emulation disabled.\n");
> @@ -580,7 +580,7 @@ static int __init numa_emulation(unsigned long start_pfn,
> memnode_shift = ret;
>
> for_each_online_node ( i )
> - setup_node_bootmem(i, nodes[i].start, nodes[i].end);
> + setup_node_bootmem(i, numa_nodes[i].start, numa_nodes[i].end);
>
> numa_init_array();
>
I think renaming the file-scope variable this way would be more logical
and less risky (the way you do it it's easy to miss one place without
the build breaking).
> --- a/xen/drivers/char/ehci-dbgp.c
> +++ b/xen/drivers/char/ehci-dbgp.c
> @@ -424,9 +424,9 @@ static void dbgp_issue_command(struct ehci_dbgp *dbgp, u32 ctrl,
> * checks to see if ACPI or some other initialization also
> * reset the EHCI debug port.
> */
> - u32 ctrl = readl(&dbgp->ehci_debug->control);
> + u32 controller = readl(&dbgp->ehci_debug->control);
Why "controller" when the field read is named "control"? Perhaps
easiest would be to drop the variablöe altogether: It's used exactly
once, ...
> - if ( ctrl & DBGP_ENABLED )
> + if ( controller & DBGP_ENABLED )
... here.
> --- a/xen/drivers/char/ns16550.c
> +++ b/xen/drivers/char/ns16550.c
> @@ -1473,7 +1473,7 @@ static enum __init serial_param_type get_token(char *token, char **value)
>
> static bool __init parse_positional(struct ns16550 *uart, char **str)
> {
> - int baud;
> + int baud_rate;
> const char *conf;
> char *name_val_pos;
>
> @@ -1504,7 +1504,7 @@ static bool __init parse_positional(struct ns16550 *uart, char **str)
> uart->baud = BAUD_AUTO;
> conf += 4;
> }
> - else if ( (baud = simple_strtoul(conf, &conf, 10)) != 0 )
> + else if ( (baud_rate = simple_strtoul(conf, &conf, 10)) != 0 )
> uart->baud = baud;
So along the lines of the earlier remark on common/numa.c: Here you're
actively introducing a bug, by not also renaming the further use of the
variable. Please reconsider the name change.
Jan
On 31/07/2023 16:34, Jan Beulich wrote:
> On 31.07.2023 15:35, 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"
>>
>> Local variables have been suitably renamed to address some violations
>> of this rule:
>> - s/cmp/c/ because it shadows the union declared at line 87.
>> - s/nodes/numa_nodes/ shadows the static variable declared at line 18.
>> - s/ctrl/controller/ because the homonymous function parameter is
>> later
>> read.
>> - s/baud/baud_rate/ to avoid shadowing the enum constant defined
>> at line 1391.
>>
>> No functional change.
>>
>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
>> ---
>> xen/common/compat/memory.c | 6 +++---
>> xen/common/numa.c | 36
>> ++++++++++++++++++------------------
>> xen/drivers/char/ehci-dbgp.c | 4 ++--
>> xen/drivers/char/ns16550.c | 4 ++--
>> 4 files changed, 25 insertions(+), 25 deletions(-)
>
> This is an odd mix of files touched in a single patch. How about
> splitting
> into two, one for common/ and one for drivers/?
>
Ok, I'll do it.
>> --- a/xen/common/compat/memory.c
>> +++ b/xen/common/compat/memory.c
>> @@ -321,12 +321,12 @@ int compat_memory_op(unsigned int cmd,
>> XEN_GUEST_HANDLE_PARAM(void) compat)
>>
>> case XENMEM_remove_from_physmap:
>> {
>> - struct compat_remove_from_physmap cmp;
>> + struct compat_remove_from_physmap c;
>
> The intention of the outer scope cmp is to avoid such inner scope
> ones then consuming extra stack space. This wants making part of the
> union there.
>
Makes sense, though I've not been able to find a definition for the type
'struct compat_remove_from_physmap'.
>> --- a/xen/common/numa.c
>> +++ b/xen/common/numa.c
>> @@ -382,7 +382,7 @@ static bool __init numa_process_nodes(paddr_t
>> start, paddr_t end)
>> * 0 if memnodmap[] too small (or shift too small)
>> * -1 if node overlap or lost ram (shift too big)
>> */
>> -static int __init populate_memnodemap(const struct node *nodes,
>> +static int __init populate_memnodemap(const struct node *numa_nodes,
>> unsigned int numnodes, unsigned
>> int shift,
>> const nodeid_t *nodeids)
>> {
>> @@ -393,8 +393,8 @@ static int __init populate_memnodemap(const struct
>> node *nodes,
>>
>> for ( i = 0; i < numnodes; i++ )
>> {
>> - unsigned long spdx = paddr_to_pdx(nodes[i].start);
>> - unsigned long epdx = paddr_to_pdx(nodes[i].end - 1);
>> + unsigned long spdx = paddr_to_pdx(numa_nodes[i].start);
>> + unsigned long epdx = paddr_to_pdx(numa_nodes[i].end - 1);
>>
>> if ( spdx > epdx )
>> continue;
>> @@ -440,7 +440,7 @@ static int __init
>> allocate_cachealigned_memnodemap(void)
>> * The LSB of all start addresses in the node map is the value of the
>> * maximum possible shift.
>> */
>> -static unsigned int __init extract_lsb_from_nodes(const struct node
>> *nodes,
>> +static unsigned int __init extract_lsb_from_nodes(const struct node
>> *numa_nodes,
>> nodeid_t numnodes,
>> const nodeid_t
>> *nodeids)
>> {
>> @@ -449,8 +449,8 @@ static unsigned int __init
>> extract_lsb_from_nodes(const struct node *nodes,
>>
>> for ( i = 0; i < numnodes; i++ )
>> {
>> - unsigned long spdx = paddr_to_pdx(nodes[i].start);
>> - unsigned long epdx = paddr_to_pdx(nodes[i].end - 1) + 1;
>> + unsigned long spdx = paddr_to_pdx(numa_nodes[i].start);
>> + unsigned long epdx = paddr_to_pdx(numa_nodes[i].end - 1) + 1;
>>
>> if ( spdx >= epdx )
>> continue;
>> @@ -475,10 +475,10 @@ static unsigned int __init
>> extract_lsb_from_nodes(const struct node *nodes,
>> return i;
>> }
>>
>> -int __init compute_hash_shift(const struct node *nodes,
>> +int __init compute_hash_shift(const struct node *numa_nodes,
>> unsigned int numnodes, const nodeid_t
>> *nodeids)
>> {
>> - unsigned int shift = extract_lsb_from_nodes(nodes, numnodes,
>> nodeids);
>> + unsigned int shift = extract_lsb_from_nodes(numa_nodes, numnodes,
>> nodeids);
>>
>> if ( memnodemapsize <= ARRAY_SIZE(_memnodemap) )
>> memnodemap = _memnodemap;
>> @@ -487,7 +487,7 @@ int __init compute_hash_shift(const struct node
>> *nodes,
>>
>> printk(KERN_DEBUG "NUMA: Using %u for the hash shift\n", shift);
>>
>> - if ( populate_memnodemap(nodes, numnodes, shift, nodeids) != 1 )
>> + if ( populate_memnodemap(numa_nodes, numnodes, shift, nodeids) !=
>> 1 )
>> {
>> printk(KERN_INFO "Your memory is not aligned you need to "
>> "rebuild your hypervisor with a bigger NODEMAPSIZE "
>> @@ -541,7 +541,7 @@ static int __init numa_emulation(unsigned long
>> start_pfn,
>> {
>> int ret;
>> unsigned int i;
>> - struct node nodes[MAX_NUMNODES];
>> + struct node numa_nodes[MAX_NUMNODES];
>> uint64_t sz = pfn_to_paddr(end_pfn - start_pfn) / numa_fake;
>>
>> /* Kludge needed for the hash function */
>> @@ -556,22 +556,22 @@ static int __init numa_emulation(unsigned long
>> start_pfn,
>> sz = x;
>> }
>>
>> - memset(&nodes, 0, sizeof(nodes));
>> + memset(&numa_nodes, 0, sizeof(numa_nodes));
>> for ( i = 0; i < numa_fake; i++ )
>> {
>> - nodes[i].start = pfn_to_paddr(start_pfn) + i * sz;
>> + numa_nodes[i].start = pfn_to_paddr(start_pfn) + i * sz;
>>
>> if ( i == numa_fake - 1 )
>> - sz = pfn_to_paddr(end_pfn) - nodes[i].start;
>> + sz = pfn_to_paddr(end_pfn) - numa_nodes[i].start;
>>
>> - nodes[i].end = nodes[i].start + sz;
>> + numa_nodes[i].end = numa_nodes[i].start + sz;
>> printk(KERN_INFO "Faking node %u at %"PRIx64"-%"PRIx64"
>> (%"PRIu64"MB)\n",
>> - i, nodes[i].start, nodes[i].end,
>> - (nodes[i].end - nodes[i].start) >> 20);
>> + i, numa_nodes[i].start, numa_nodes[i].end,
>> + (numa_nodes[i].end - numa_nodes[i].start) >> 20);
>> node_set_online(i);
>> }
>>
>> - ret = compute_hash_shift(nodes, numa_fake, NULL);
>> + ret = compute_hash_shift(numa_nodes, numa_fake, NULL);
>> if ( ret < 0 )
>> {
>> printk(KERN_ERR "No NUMA hash function found. Emulation
>> disabled.\n");
>> @@ -580,7 +580,7 @@ static int __init numa_emulation(unsigned long
>> start_pfn,
>> memnode_shift = ret;
>>
>> for_each_online_node ( i )
>> - setup_node_bootmem(i, nodes[i].start, nodes[i].end);
>> + setup_node_bootmem(i, numa_nodes[i].start,
>> numa_nodes[i].end);
>>
>> numa_init_array();
>>
>
> I think renaming the file-scope variable this way would be more logical
> and less risky (the way you do it it's easy to miss one place without
> the build breaking).
>
Ok
>> --- a/xen/drivers/char/ehci-dbgp.c
>> +++ b/xen/drivers/char/ehci-dbgp.c
>> @@ -424,9 +424,9 @@ static void dbgp_issue_command(struct ehci_dbgp
>> *dbgp, u32 ctrl,
>> * checks to see if ACPI or some other initialization also
>> * reset the EHCI debug port.
>> */
>> - u32 ctrl = readl(&dbgp->ehci_debug->control);
>> + u32 controller = readl(&dbgp->ehci_debug->control);
>
> Why "controller" when the field read is named "control"? Perhaps
> easiest would be to drop the variablöe altogether: It's used exactly
> once, ...
>
>> - if ( ctrl & DBGP_ENABLED )
>> + if ( controller & DBGP_ENABLED )
>
> ... here.
>
I was fooled by the comment above its declaration. At first sight I
don't
see anything wrong with dropping the variable, but I'll check it
properly before submitting
a v2.
>> --- a/xen/drivers/char/ns16550.c
>> +++ b/xen/drivers/char/ns16550.c
>> @@ -1473,7 +1473,7 @@ static enum __init serial_param_type
>> get_token(char *token, char **value)
>>
>> static bool __init parse_positional(struct ns16550 *uart, char **str)
>> {
>> - int baud;
>> + int baud_rate;
>> const char *conf;
>> char *name_val_pos;
>>
>> @@ -1504,7 +1504,7 @@ static bool __init parse_positional(struct
>> ns16550 *uart, char **str)
>> uart->baud = BAUD_AUTO;
>> conf += 4;
>> }
>> - else if ( (baud = simple_strtoul(conf, &conf, 10)) != 0 )
>> + else if ( (baud_rate = simple_strtoul(conf, &conf, 10)) != 0 )
>> uart->baud = baud;
>
> So along the lines of the earlier remark on common/numa.c: Here you're
> actively introducing a bug, by not also renaming the further use of the
> variable. Please reconsider the name change.
>
Good catch, I'll do as suggested in v2.
--
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)
On 01.08.2023 09:20, Nicola Vetrini wrote:
> On 31/07/2023 16:34, Jan Beulich wrote:
>> On 31.07.2023 15:35, Nicola Vetrini wrote:
>>> --- a/xen/common/compat/memory.c
>>> +++ b/xen/common/compat/memory.c
>>> @@ -321,12 +321,12 @@ int compat_memory_op(unsigned int cmd,
>>> XEN_GUEST_HANDLE_PARAM(void) compat)
>>>
>>> case XENMEM_remove_from_physmap:
>>> {
>>> - struct compat_remove_from_physmap cmp;
>>> + struct compat_remove_from_physmap c;
>>
>> The intention of the outer scope cmp is to avoid such inner scope
>> ones then consuming extra stack space. This wants making part of the
>> union there.
>>
>
> Makes sense, though I've not been able to find a definition for the type
> 'struct compat_remove_from_physmap'.
This is a generated type, so you'll need to look in the build tree
under xen/include/compat/. (In this context I'm curious how Misra and
Eclair deal with generated code.)
Jan
© 2016 - 2026 Red Hat, Inc.