Re: [Xen-devel] [PATCH v6 10/12] xen/arm: move arch specific grant table bits into grant_table.c
Hi Juergen, On 13/09/17 16:46, Juergen Gross wrote: diff --git a/xen/include/xen/grant_table.h b/xen/include/xen/grant_table.h index df11b31264..f3f2fb9ebc 100644 --- a/xen/include/xen/grant_table.h +++ b/xen/include/xen/grant_table.h @@ -27,7 +27,8 @@ #include #include #include -#include This change looks a bit strange to me. This is the only place where asm/grant_table.h is pulled. Because you remove it, it now means that the prototype will not be defined first and may result to mismatch in the future. Ideally we should enforce, although it would require some work as we didn't really follow that rule in a few places. Cheers, + +struct grant_table; /* The maximum size of a grant table. */ extern unsigned int max_grant_frames; -- Julien Grall ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v6 10/12] xen/arm: move arch specific grant table bits into grant_table.c
On 14/09/17 19:31, Julien Grall wrote: > Hi Juergen, > > On 13/09/17 16:46, Juergen Gross wrote: >> diff --git a/xen/include/xen/grant_table.h >> b/xen/include/xen/grant_table.h >> index df11b31264..f3f2fb9ebc 100644 >> --- a/xen/include/xen/grant_table.h >> +++ b/xen/include/xen/grant_table.h >> @@ -27,7 +27,8 @@ >> #include >> #include >> #include >> -#include > > This change looks a bit strange to me. This is the only place where > asm/grant_table.h is pulled. Because you remove it, it now means that > the prototype will not be defined first and may result to mismatch in > the future. > > Ideally we should enforce, although it would require some work as we > didn't really follow that rule in a few places. Aah, I missed the prototypes which relate to functions outside of grant_table.c. Strange that the compiler didn't barf at me. All those functions seem to be rather small, so I can add them as inline to asm-arm/grant_table.h, as they are used only by grant_table.c. Juergen ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v6 10/12] xen/arm: move arch specific grant table bits into grant_table.c
Hi Juergen, On 15/09/2017 08:22, Juergen Gross wrote: On 14/09/17 19:31, Julien Grall wrote: Hi Juergen, On 13/09/17 16:46, Juergen Gross wrote: diff --git a/xen/include/xen/grant_table.h b/xen/include/xen/grant_table.h index df11b31264..f3f2fb9ebc 100644 --- a/xen/include/xen/grant_table.h +++ b/xen/include/xen/grant_table.h @@ -27,7 +27,8 @@ #include #include #include -#include This change looks a bit strange to me. This is the only place where asm/grant_table.h is pulled. Because you remove it, it now means that the prototype will not be defined first and may result to mismatch in the future. Ideally we should enforce, although it would require some work as we didn't really follow that rule in a few places. Aah, I missed the prototypes which relate to functions outside of grant_table.c. Strange that the compiler didn't barf at me. It is because we don't set -Wmissing-prototypes. This would break compilation on Xen because of quite a few missing prototypes within the source code. All those functions seem to be rather small, so I can add them as inline to asm-arm/grant_table.h, as they are used only by grant_table.c. I think you would still need to include asm/grant_table.h for x86 to get the prototype defined for create_grant_p2m_mapping & co. To be honest, I am not sure to fully understand the rationale to not include asm/grant_table.h in grant_table.h. Cheers, -- Julien Grall ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v6 10/12] xen/arm: move arch specific grant table bits into grant_table.c
On 15/09/17 10:49, Julien Grall wrote: > Hi Juergen, > > On 15/09/2017 08:22, Juergen Gross wrote: >> On 14/09/17 19:31, Julien Grall wrote: >>> Hi Juergen, >>> >>> On 13/09/17 16:46, Juergen Gross wrote: diff --git a/xen/include/xen/grant_table.h b/xen/include/xen/grant_table.h index df11b31264..f3f2fb9ebc 100644 --- a/xen/include/xen/grant_table.h +++ b/xen/include/xen/grant_table.h @@ -27,7 +27,8 @@ #include #include #include -#include >>> >>> This change looks a bit strange to me. This is the only place where >>> asm/grant_table.h is pulled. Because you remove it, it now means that >>> the prototype will not be defined first and may result to mismatch in >>> the future. >>> >>> Ideally we should enforce, although it would require some work as we >>> didn't really follow that rule in a few places. >> >> Aah, I missed the prototypes which relate to functions outside of >> grant_table.c. Strange that the compiler didn't barf at me. > > It is because we don't set -Wmissing-prototypes. This would break > compilation on Xen because of quite a few missing prototypes within the > source code. Uuh, I wasn't aware of this! >> All those functions seem to be rather small, so I can add them as inline >> to asm-arm/grant_table.h, as they are used only by grant_table.c. > > I think you would still need to include asm/grant_table.h for x86 to get > the prototype defined for create_grant_p2m_mapping & co. > > To be honest, I am not sure to fully understand the rationale to not > include asm/grant_table.h in grant_table.h. The idea was that asm/grant_table.h only contains abstractions needed in grant_table.c. Why include it in a Xen-global header when only one source needs its contents? Additionally when I started this patch I tried to use inline functions instead of #defines in asm/grant-table.h leading to compile errors when struct grant_table was dereferenced. But with the missing prototypes problem I think it might really be better to keep the #include in grant_table.h. Juergen ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
