| Message ID | aeafaee0fc4a507f6ba0c10e8fed90ed73a6bd6d.1701344917.git.federico.serafini@bugseng.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/p2m: address a violation of MISRA C:2012 Rule 8.3 | expand |
On 30.11.2023 16:48, Federico Serafini wrote: > The objective is to use parameter name "gfn" for > xenmem_add_to_physmap_one(). > Since the name "gfn" is currently used as identifier for a local > variable, bad things could happen if new uses of such variable are > committed while a renaming patch is waiting for the approval. > To avoid such danger, as first thing rename the local variable from > "gfn" to "gmfn". "..., in line with XENMAPSPACE_gmfn which is the only case it is used with." This is to justify the name not matching our generally aimed at "gfn" and "mfn" scheme. > No functional change. > > Signed-off-by: Federico Serafini <federico.serafini@bugseng.com> Reviewed-by: Jan Beulich <jbeulich@suse.com> Jan
On 04/12/23 15:51, Jan Beulich wrote: > On 30.11.2023 16:48, Federico Serafini wrote: >> The objective is to use parameter name "gfn" for >> xenmem_add_to_physmap_one(). >> Since the name "gfn" is currently used as identifier for a local >> variable, bad things could happen if new uses of such variable are >> committed while a renaming patch is waiting for the approval. >> To avoid such danger, as first thing rename the local variable from >> "gfn" to "gmfn". > > "..., in line with XENMAPSPACE_gmfn which is the only case it is used > with." > > This is to justify the name not matching our generally aimed at "gfn" > and "mfn" scheme. > >> No functional change. >> >> Signed-off-by: Federico Serafini <federico.serafini@bugseng.com> > > Reviewed-by: Jan Beulich <jbeulich@suse.com> There is an use of "gfn" also few lines outside of the switch statement, within an if condition where also XENMAPSPACE_gmfn is involved: what is true is that "gfn" is used only when space == XENMAPSPACE_gmfn. What do you think about improve the description by adding: "..., in line with XENMAPSPACE_gmfn which is the only *space* it is used with." However, the description improvement can be done on commit?
On 04.12.2023 16:42, Federico Serafini wrote: > On 04/12/23 15:51, Jan Beulich wrote: >> On 30.11.2023 16:48, Federico Serafini wrote: >>> The objective is to use parameter name "gfn" for >>> xenmem_add_to_physmap_one(). >>> Since the name "gfn" is currently used as identifier for a local >>> variable, bad things could happen if new uses of such variable are >>> committed while a renaming patch is waiting for the approval. >>> To avoid such danger, as first thing rename the local variable from >>> "gfn" to "gmfn". >> >> "..., in line with XENMAPSPACE_gmfn which is the only case it is used >> with." >> >> This is to justify the name not matching our generally aimed at "gfn" >> and "mfn" scheme. >> >>> No functional change. >>> >>> Signed-off-by: Federico Serafini <federico.serafini@bugseng.com> >> >> Reviewed-by: Jan Beulich <jbeulich@suse.com> > > There is an use of "gfn" also few lines outside of the > switch statement, within an if condition where also XENMAPSPACE_gmfn is > involved: > what is true is that "gfn" is used only when space == XENMAPSPACE_gmfn. Well, sure - me saying "case" wasn't meant to limit things to the switch() statement. > What do you think about improve the description by adding: > "..., in line with XENMAPSPACE_gmfn which is the only *space* it is used > with." Fine with me. > However, the description improvement can be done on commit? It can. Nevertheless you want to avoid getting into the habit of asking for things to be done while committing. Strictly speaking on-commit editing isn't entirely correct, as committing ought to be a purely mechanical operation. In how far a particular committer is willing to deviate from that should be left to them. Jan
diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c index fe9ccabb87..42508e1e7e 100644 --- a/xen/arch/x86/mm/p2m.c +++ b/xen/arch/x86/mm/p2m.c @@ -2417,7 +2417,7 @@ int xenmem_add_to_physmap_one( gfn_t gpfn) { struct page_info *page = NULL; - unsigned long gfn = 0 /* gcc ... */, old_gpfn; + unsigned long gmfn = 0 /* gcc ... */, old_gpfn; mfn_t prev_mfn; int rc = 0; mfn_t mfn = INVALID_MFN; @@ -2440,12 +2440,12 @@ int xenmem_add_to_physmap_one( case XENMAPSPACE_gmfn: { - gfn = idx; - mfn = get_gfn_unshare(d, gfn, &p2mt); + gmfn = idx; + mfn = get_gfn_unshare(d, gmfn, &p2mt); /* If the page is still shared, exit early */ if ( p2m_is_shared(p2mt) ) { - put_gfn(d, gfn); + put_gfn(d, gmfn); return -ENOMEM; } page = get_page_from_mfn(mfn, d); @@ -2480,7 +2480,7 @@ int xenmem_add_to_physmap_one( /* XENMAPSPACE_gmfn: Check if the MFN is associated with another GFN. */ old_gpfn = get_gpfn_from_mfn(mfn_x(mfn)); ASSERT(!SHARED_M2P(old_gpfn)); - if ( space == XENMAPSPACE_gmfn && old_gpfn != gfn ) + if ( space == XENMAPSPACE_gmfn && old_gpfn != gmfn ) { rc = -EXDEV; goto put_all; @@ -2518,7 +2518,7 @@ int xenmem_add_to_physmap_one( */ if ( space == XENMAPSPACE_gmfn ) { - put_gfn(d, gfn); + put_gfn(d, gmfn); if ( !rc && extra.ppage ) { *extra.ppage = page;
The objective is to use parameter name "gfn" for xenmem_add_to_physmap_one(). Since the name "gfn" is currently used as identifier for a local variable, bad things could happen if new uses of such variable are committed while a renaming patch is waiting for the approval. To avoid such danger, as first thing rename the local variable from "gfn" to "gmfn". No functional change. Signed-off-by: Federico Serafini <federico.serafini@bugseng.com> --- xen/arch/x86/mm/p2m.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)