Re: [PATCH 4/4] xen/public: Use -Wpadding for public headers

[Jan Beulich <jbeulich@xxxxxxxx>]

On 15.04.2024 17:41, Andrew Cooper wrote:
> RFC.  In theory this is a great way to avoid some of the spiketraps involved
> with C being the official representation.
> 
> However, this doesn't build.  gnttab_transfer has a layout that requires a
> CONFIG_COMPAT if we want to satisfy -Wpadding for both forms of the structure.
> 
> Thoughts on whether this cross-check is worthwhile-enough to warrant the
> ifdefary?

#ifdef-ary in general would be okay. But any #ifdef CONFIG_* would look pretty
odd to me in a public header. Perhaps as

#if defined(__XEN__) && defined(CONFIG_COMPAT)

it might be tolerable.

> --- /dev/null
> +++ b/xen/common/hdr-chk.c
> @@ -0,0 +1,13 @@
> +#include <xen/stdint.h>
> +
> +#include <public/xen.h>
> +
> +#pragma GCC diagnostic error "-Wpadded"

Everywhere up to here you say -Wpadding.

> +#include <public/grant_table.h>
> +
> +#ifdef CONFIG_COMPAT
> +
> +#include <compat/grant_table.h>
> +
> +#endif /* CONFIG_COMPAT */

I'm not overly happy to see a 2nd header checking "pass" added. We already
have the headers.chk goal in xen/include/Makefile, after all. For the non-
generated headers adding -Wpadded there would seem more natural to me,
first and foremost because then it is less likely that one of the two places
would be missed if a new header is added. Something long those lines may then
need adding for the generated compat headers, but again preferably without
enumerating them all in yet another place.

> --- a/xen/include/public/grant_table.h
> +++ b/xen/include/public/grant_table.h
> @@ -355,6 +355,7 @@ struct gnttab_unmap_grant_ref {
>      grant_handle_t handle;
>      /* OUT parameters. */
>      int16_t  status;              /* => enum grant_status */
> +    uint16_t _pad0;

While you may view it as nitpicking, in the public headers I'm pretty firm
on not wanting to see new name space violations, i.e. new names with leading
underscores which aren't file-scope identifiers.

Furthermore what's the deal with using "pad0" here and in one more place,
but "ign1" / "ign2" in other cases?

> @@ -371,6 +372,7 @@ DEFINE_XEN_GUEST_HANDLE(gnttab_unmap_grant_ref_t);
>  struct gnttab_setup_table {
>      /* IN parameters. */
>      domid_t  dom;
> +    uint16_t _pad0;
>      uint32_t nr_frames;
>      /* OUT parameters. */
>      int16_t  status;              /* => enum grant_status */

I'm surprised no padding field would be needed right below here, seeing
that what follows is a handle:

#if __XEN_INTERFACE_VERSION__ < 0x00040300
    XEN_GUEST_HANDLE(ulong) frame_list;
#else
    XEN_GUEST_HANDLE(xen_pfn_t) frame_list;
#endif

The size of this padding field would then also be compat-dependent, I
suppose.

Jan