| Message ID | 1457196754-6299-1-git-send-email-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 3/5/16 10:52 AM, Andrew Cooper wrote: > If an architecture does not provide a custom page_list_entry, default > page_list_* helpers are provided, wrapping list_head as an underlying type for > page_list_head. > > The two declarations of the page_list_* helpers differ between defines and > static inline functions, where the defines discard some of their parameters. > > This causes a compilation failure if CONFIG_BIGMEM and debug=n in p2m-pod.c: > > p2m-pod.c: In function ‘p2m_pod_cache_add’: > p2m-pod.c:72:20: error: unused variable ‘d’ [-Werror=unused-variable] > struct domain *d = p2m->domain; > ^ > cc1: all warnings being treated as errors > > because the use of d outside of the !NDEBUG section doesn't get evaluated as a > parameter by page_list_del(). > > Fix this by turning all #defines into static inline functions, so all > parameters are evaluated even if they are not used. > > Doing this reveals that const-correctness of page_list_{next,prev}() is > suspect, taking a const pointer and returning a non-const one. It is left > functioning as it did before, with an explicit typecast to remove constness. > > While editing this area, correct the return type of page_list_empty from int > to bool_t. > > Reported-by: Doug Goldstein <cardoe@cardoe.com> > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> Reviewed-by: Doug Goldstein <cardoe@cardoe.com> I had planned on doing something similar but your commit message is much better than what I would have come up with. Not sure if people would like Identified-by: Travis CI but it actually required a patch to be found. http://lists.xen.org/archives/html/xen-devel/2016-03/msg00735.html which is waiting to be merged.
>>> On 05.03.16 at 17:52, <andrew.cooper3@citrix.com> wrote: > Doing this reveals that const-correctness of page_list_{next,prev}() is > suspect, taking a const pointer and returning a non-const one. It is left > functioning as it did before, with an explicit typecast to remove constness. I don't see anything suspect here: Retrieving the next or prev list element doesn't alter the current one, so the input pointer can legitimately be const, while the output one obviously shouldn't be. Or else why don't you similarly consider page_list_{first,last}() bogus in that same respect? > +static inline bool_t > +page_list_empty(const struct page_list_head *head) > +{ > + return list_empty(head); > +} While I appreciate the conversion to bool_t, to be fully correct you either need to use !! here, or switch list_empty() to bool_t too. > +static inline struct page_info * > +page_list_first(const struct page_list_head *head) > +{ > + return list_first_entry(head, struct page_info, list); > +} > +static inline struct page_info * > +page_list_last(const struct page_list_head *head) > +{ > + return list_last_entry(head, struct page_info, list); > +} > +static inline struct page_info * > +page_list_next(const struct page_info *page, > + const struct page_list_head *head) > +{ > + return list_next_entry((struct page_info *)page, list); > +} > +static inline struct page_info * > +page_list_prev(const struct page_info *page, > + const struct page_list_head *head) > +{ > + return list_prev_entry((struct page_info *)page, list); > +} I'd suggest to avoid the explicit casts, by using list_entry(page->list.next, struct page_info, list) and list_entry(page->list.prev, struct page_info, list) respectively. Jan
diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h index a795dd6..38e12b7 100644 --- a/xen/include/xen/mm.h +++ b/xen/include/xen/mm.h @@ -220,7 +220,7 @@ struct page_list_head # define INIT_PAGE_LIST_HEAD(head) ((head)->tail = (head)->next = NULL) # define INIT_PAGE_LIST_ENTRY(ent) ((ent)->prev = (ent)->next = PAGE_LIST_NULL) -static inline int +static inline bool_t page_list_empty(const struct page_list_head *head) { return !head->next; @@ -392,31 +392,84 @@ page_list_splice(struct page_list_head *list, struct page_list_head *head) # define PAGE_LIST_HEAD LIST_HEAD # define INIT_PAGE_LIST_HEAD INIT_LIST_HEAD # define INIT_PAGE_LIST_ENTRY INIT_LIST_HEAD -# define page_list_empty list_empty -# define page_list_first(hd) \ - list_first_entry(hd, struct page_info, list) -# define page_list_last(hd) \ - list_last_entry(hd, struct page_info, list) -# define page_list_next(pg, hd) list_next_entry(pg, list) -# define page_list_prev(pg, hd) list_prev_entry(pg, list) -# define page_list_add(pg, hd) list_add(&(pg)->list, hd) -# define page_list_add_tail(pg, hd) list_add_tail(&(pg)->list, hd) -# define page_list_del(pg, hd) list_del(&(pg)->list) -# define page_list_del2(pg, hd1, hd2) list_del(&(pg)->list) -# define page_list_remove_head(hd) (!page_list_empty(hd) ? \ - ({ \ - struct page_info *__pg = page_list_first(hd); \ - list_del(&__pg->list); \ - __pg; \ - }) : NULL) -# define page_list_move(dst, src) (!list_empty(src) ? \ - list_replace_init(src, dst) : (void)0) + +static inline bool_t +page_list_empty(const struct page_list_head *head) +{ + return list_empty(head); +} +static inline struct page_info * +page_list_first(const struct page_list_head *head) +{ + return list_first_entry(head, struct page_info, list); +} +static inline struct page_info * +page_list_last(const struct page_list_head *head) +{ + return list_last_entry(head, struct page_info, list); +} +static inline struct page_info * +page_list_next(const struct page_info *page, + const struct page_list_head *head) +{ + return list_next_entry((struct page_info *)page, list); +} +static inline struct page_info * +page_list_prev(const struct page_info *page, + const struct page_list_head *head) +{ + return list_prev_entry((struct page_info *)page, list); +} +static inline void +page_list_add(struct page_info *page, struct page_list_head *head) +{ + list_add(&page->list, head); +} +static inline void +page_list_add_tail(struct page_info *page, struct page_list_head *head) +{ + list_add_tail(&page->list, head); +} +static inline void +page_list_del(struct page_info *page, struct page_list_head *head) +{ + list_del(&page->list); +} +static inline void +page_list_del2(struct page_info *page, struct page_list_head *head1, + struct page_list_head *head2) +{ + list_del(&page->list); +} +static inline struct page_info * +page_list_remove_head(struct page_list_head *head) +{ + struct page_info *pg; + + if ( page_list_empty(head) ) + return NULL; + + pg = page_list_first(head); + list_del(&pg->list); + return pg; +} +static inline void +page_list_move(struct page_list_head *dst, struct page_list_head *src) +{ + if ( !list_empty(src) ) + list_replace_init(src, dst); +} +static inline void +page_list_splice(struct page_list_head *list, struct page_list_head *head) +{ + list_splice(list, head); +} + # define page_list_for_each(pos, head) list_for_each_entry(pos, head, list) # define page_list_for_each_safe(pos, tmp, head) \ list_for_each_entry_safe(pos, tmp, head, list) # define page_list_for_each_safe_reverse(pos, tmp, head) \ list_for_each_entry_safe_reverse(pos, tmp, head, list) -# define page_list_splice(list, hd) list_splice(list, hd) #endif static inline unsigned int get_order_from_bytes(paddr_t size)
If an architecture does not provide a custom page_list_entry, default page_list_* helpers are provided, wrapping list_head as an underlying type for page_list_head. The two declarations of the page_list_* helpers differ between defines and static inline functions, where the defines discard some of their parameters. This causes a compilation failure if CONFIG_BIGMEM and debug=n in p2m-pod.c: p2m-pod.c: In function ‘p2m_pod_cache_add’: p2m-pod.c:72:20: error: unused variable ‘d’ [-Werror=unused-variable] struct domain *d = p2m->domain; ^ cc1: all warnings being treated as errors because the use of d outside of the !NDEBUG section doesn't get evaluated as a parameter by page_list_del(). Fix this by turning all #defines into static inline functions, so all parameters are evaluated even if they are not used. Doing this reveals that const-correctness of page_list_{next,prev}() is suspect, taking a const pointer and returning a non-const one. It is left functioning as it did before, with an explicit typecast to remove constness. While editing this area, correct the return type of page_list_empty from int to bool_t. Reported-by: Doug Goldstein <cardoe@cardoe.com> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Jan Beulich <JBeulich@suse.com> CC: Tim Deegan <tim@xen.org> CC: George Dunlap <george.dunlap@eu.citrix.com> --- xen/include/xen/mm.h | 95 ++++++++++++++++++++++++++++++++++++++++------------ 1 file changed, 74 insertions(+), 21 deletions(-)