On 17.09.2021 04:48, Penny Zheng wrote: > Commit 540a637c3410780b519fc055f432afe271f642f8 defines a new > helper mark_page_free to extract common codes, but it broke the > local variable "tainted", revealed by Coverity ID 1491872.
Instead of mentioning the ID here, please ... > This patch let mark_page_free() return boolean value of variable > "tainted" and rename local variable "tainted" to "pg_tainted" > to tell the difference from the global variable of the same name. add a tag-like instance here, as we do elsewhere: Coverity ID: 1491872 > Signed-off-by: Penny Zheng <[email protected]> > --- > xen/common/page_alloc.c | 15 ++++++++++----- > 1 file changed, 10 insertions(+), 5 deletions(-) > > diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c > index b9441cb06f..c6f48f7a97 100644 > --- a/xen/common/page_alloc.c > +++ b/xen/common/page_alloc.c > @@ -1380,8 +1380,10 @@ bool scrub_free_pages(void) > return node_to_scrub(false) != NUMA_NO_NODE; > } > > -static void mark_page_free(struct page_info *pg, mfn_t mfn) > +static bool mark_page_free(struct page_info *pg, mfn_t mfn) > { > + unsigned int pg_tainted = 0; bool please, considering ... > @@ -1405,7 +1407,7 @@ static void mark_page_free(struct page_info *pg, mfn_t > mfn) > case PGC_state_offlining: > pg->count_info = (pg->count_info & PGC_broken) | > PGC_state_offlined; > - tainted = 1; > + pg_tainted = 1; > break; > > default: > @@ -1425,6 +1427,8 @@ static void mark_page_free(struct page_info *pg, mfn_t > mfn) > /* This page is not a guest frame any more. */ > page_set_owner(pg, NULL); /* set_gpfn_from_mfn snoops pg owner */ > set_gpfn_from_mfn(mfn_x(mfn), INVALID_M2P_ENTRY); > + > + return pg_tainted; > } ... this. Also both here and ... > @@ -1433,7 +1437,7 @@ static void free_heap_pages( > { > unsigned long mask; > mfn_t mfn = page_to_mfn(pg); > - unsigned int i, node = phys_to_nid(mfn_to_maddr(mfn)), tainted = 0; > + unsigned int i, node = phys_to_nid(mfn_to_maddr(mfn)), pg_tainted = 0; ... here I don't see why you stick to using "tainted" in the name when the goal is to ... > @@ -1517,7 +1522,7 @@ static void free_heap_pages( > > page_list_add_scrub(pg, node, zone, order, pg->u.free.first_dirty); > > - if ( tainted ) > + if ( pg_tainted ) > reserve_offlined_page(pg); ... control the call to this function: "offline" or "offlined" would seem quite a bit more to the point, getting us further away from the global meaning of "tainted". Also please follow patch submission rules: To the list, with (all) maintainers Cc-ed. Jan
