On Wed, Jul 15, 2020 at 08:36:10AM +0200, Jan Beulich wrote:
> On 14.07.2020 16:29, Roger Pau Monné wrote:
> > On Wed, Jul 01, 2020 at 12:27:37PM +0200, Jan Beulich wrote:
> >> The original intention was to ignore padding fields, but the pattern
> >> matched only ones whose names started with an underscore. Also match
> >> fields whose names are in line with the C spec by not having a leading
> >> underscore. (Note that the leading ^ in the sed regexps was pointless
> >> and hence get dropped.)
> >>
> >> This requires adjusting some vNUMA macros, to avoid triggering
> >> "enumeration value ... not handled in switch" warnings, which - due to
> >> -Werror - would cause the build to fail. (I have to admit that I find
> >> these padding fields odd, when translation of the containing structure
> >> is needed anyway.)
> >>
> >> Signed-off-by: Jan Beulich <jbeul...@suse.com>
> >
> > Reviewed-by: Roger Pau Monné <roger....@citrix.com>
>
> Thanks.
>
> >> ---
> >> While for translation macros skipping padding fields pretty surely is a
> >> reasonable thing to do, we may want to consider not ignoring them when
> >> generating checking macros.
>
> (note this remark, towards your question at the end)
>
> >> --- a/xen/common/compat/memory.c
> >> +++ b/xen/common/compat/memory.c
> >> @@ -354,10 +354,13 @@ int compat_memory_op(unsigned int cmd, X
> >> return -EFAULT;
> >>
> >> #define XLAT_vnuma_topology_info_HNDL_vdistance_h(_d_, _s_)
> >> \
> >> + case XLAT_vnuma_topology_info_vdistance_pad: \
> >> guest_from_compat_handle((_d_)->vdistance.h,
> >> (_s_)->vdistance.h)
> >> #define XLAT_vnuma_topology_info_HNDL_vcpu_to_vnode_h(_d_, _s_)
> >> \
> >> + case XLAT_vnuma_topology_info_vcpu_to_vnode_pad: \
> >> guest_from_compat_handle((_d_)->vcpu_to_vnode.h,
> >> (_s_)->vcpu_to_vnode.h)
> >> #define XLAT_vnuma_topology_info_HNDL_vmemrange_h(_d_, _s_)
> >> \
> >> + case XLAT_vnuma_topology_info_vmemrange_pad: \
> >> guest_from_compat_handle((_d_)->vmemrange.h,
> >> (_s_)->vmemrange.h)
> >
> > I find this quite ugly, would it be better to just handle them with a
> > default case in the XLAT_ macros?
>
> Default cases explicitly do not get added to be able to spot missing
> case labels, as most compilers will warn about such when the controlling
> expression is of enum type.
As you say on the comment above, ignoring those for translation
macros would be better, and would avoid the ugliness of having to add
the _pad cases here.
> > AFAICT it will also set (_d_)->vmemrange.h twice?
>
> I'm not seeing it (and if it was, I'd then also wonder why not for the
> other two handles above). This is the generated macro:
>
> #define XLAT_vnuma_topology_info(_d_, _s_) do { \
> (_d_)->domid = (_s_)->domid; \
> (_d_)->nr_vnodes = (_s_)->nr_vnodes; \
> (_d_)->nr_vcpus = (_s_)->nr_vcpus; \
> (_d_)->nr_vmemranges = (_s_)->nr_vmemranges; \
> switch (vdistance) { \
> case XLAT_vnuma_topology_info_vdistance_h: \
> XLAT_vnuma_topology_info_HNDL_vdistance_h(_d_, _s_); \
> break; \
> } \
> switch (vcpu_to_vnode) { \
> case XLAT_vnuma_topology_info_vcpu_to_vnode_h: \
> XLAT_vnuma_topology_info_HNDL_vcpu_to_vnode_h(_d_, _s_); \
> break; \
> } \
> switch (vmemrange) { \
> case XLAT_vnuma_topology_info_vmemrange_h: \
> XLAT_vnuma_topology_info_HNDL_vmemrange_h(_d_, _s_); \
> break; \
> } \
> } while (0)
>
> Am I overlooking any further aspect?
No, vdistance, vcpu_to_vnode and vmemrange are set by the caller, so
the enums will never have the _pad value, and hence the assignation
will be done only once, you are right.
Thanks, Roger.
