| Message ID | 20240216140853.5213-1-dkirjanov@suse.de (mailing list archive) |
|---|---|
| State | Rejected, archived |
| Delegated to: | Michal Kubecek |
| Headers | show |
| Series | [ethtool] move variable-sized members to the end of structs | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
On Fri, Feb 16, 2024 at 09:08:53AM -0500, Denis Kirjanov wrote: > The patch fixes the following clang warnings: > > warning: field 'xxx' with variable sized type 'xxx' not at the end of a struct > or class is a GNU extension [-Wgnu-variable-sized-type-not-at-end] > > Signed-off-by: Denis Kirjanov <dkirjanov@suse.de> Have you checked if this does not break the ioctl interface? Many of these structures (maybe even all of them) are directly passed to kernel via ioctl so that rearranging them would break the data block format expected by kernel. AFAICS at least in the cases that I checked, the outer struct member is exactly the data expected as variable part of the inner struct. Michal > --- > ethtool.c | 18 +++++++++--------- > internal.h | 2 +- > 2 files changed, 10 insertions(+), 10 deletions(-) > > diff --git a/ethtool.c b/ethtool.c > index 3ac15a7..32e79ae 100644 > --- a/ethtool.c > +++ b/ethtool.c > @@ -1736,8 +1736,8 @@ get_stringset(struct cmd_context *ctx, enum ethtool_stringset set_id, > ptrdiff_t drvinfo_offset, int null_terminate) > { > struct { > - struct ethtool_sset_info hdr; > u32 buf[1]; > + struct ethtool_sset_info hdr; > } sset_info; > struct ethtool_drvinfo drvinfo; > u32 len, i; > @@ -2683,8 +2683,8 @@ do_ioctl_glinksettings(struct cmd_context *ctx) > { > int err; > struct { > - struct ethtool_link_settings req; > __u32 link_mode_data[3 * ETHTOOL_LINK_MODE_MASK_MAX_KERNEL_NU32]; > + struct ethtool_link_settings req; > } ecmd; > struct ethtool_link_usettings *link_usettings; > unsigned int u32_offs; > @@ -2752,8 +2752,8 @@ do_ioctl_slinksettings(struct cmd_context *ctx, > const struct ethtool_link_usettings *link_usettings) > { > struct { > - struct ethtool_link_settings req; > __u32 link_mode_data[3 * ETHTOOL_LINK_MODE_MASK_MAX_KERNEL_NU32]; > + struct ethtool_link_settings req; > } ecmd; > unsigned int u32_offs; > > @@ -5206,8 +5206,8 @@ static int do_get_phy_tunable(struct cmd_context *ctx) > > if (!strcmp(argp[0], "downshift")) { > struct { > - struct ethtool_tunable ds; > u8 count; > + struct ethtool_tunable ds; > } cont; > > cont.ds.cmd = ETHTOOL_PHY_GTUNABLE; > @@ -5224,8 +5224,8 @@ static int do_get_phy_tunable(struct cmd_context *ctx) > fprintf(stdout, "Downshift disabled\n"); > } else if (!strcmp(argp[0], "fast-link-down")) { > struct { > - struct ethtool_tunable fld; > u8 msecs; > + struct ethtool_tunable fld; > } cont; > > cont.fld.cmd = ETHTOOL_PHY_GTUNABLE; > @@ -5246,8 +5246,8 @@ static int do_get_phy_tunable(struct cmd_context *ctx) > cont.msecs); > } else if (!strcmp(argp[0], "energy-detect-power-down")) { > struct { > - struct ethtool_tunable ds; > u16 msecs; > + struct ethtool_tunable ds; > } cont; > > cont.ds.cmd = ETHTOOL_PHY_GTUNABLE; > @@ -5494,8 +5494,8 @@ static int do_set_phy_tunable(struct cmd_context *ctx) > /* Do it */ > if (ds_changed) { > struct { > - struct ethtool_tunable ds; > u8 count; > + struct ethtool_tunable ds; > } cont; > > cont.ds.cmd = ETHTOOL_PHY_STUNABLE; > @@ -5510,8 +5510,8 @@ static int do_set_phy_tunable(struct cmd_context *ctx) > } > } else if (fld_changed) { > struct { > - struct ethtool_tunable fld; > u8 msecs; > + struct ethtool_tunable fld; > } cont; > > cont.fld.cmd = ETHTOOL_PHY_STUNABLE; > @@ -5526,8 +5526,8 @@ static int do_set_phy_tunable(struct cmd_context *ctx) > } > } else if (edpd_changed) { > struct { > - struct ethtool_tunable fld; > u16 msecs; > + struct ethtool_tunable fld; > } cont; > > cont.fld.cmd = ETHTOOL_PHY_STUNABLE; > diff --git a/internal.h b/internal.h > index 4b994f5..e0beec6 100644 > --- a/internal.h > +++ b/internal.h > @@ -152,12 +152,12 @@ struct ethtool_link_usettings { > struct { > __u8 transceiver; > } deprecated; > - struct ethtool_link_settings base; > struct { > ETHTOOL_DECLARE_LINK_MODE_MASK(supported); > ETHTOOL_DECLARE_LINK_MODE_MASK(advertising); > ETHTOOL_DECLARE_LINK_MODE_MASK(lp_advertising); > } link_modes; > + struct ethtool_link_settings base; > }; > > #define ethtool_link_mode_for_each_u32(index) \ > -- > 2.30.2 >
On 2/16/2024 6:57 AM, Michal Kubecek wrote: > On Fri, Feb 16, 2024 at 09:08:53AM -0500, Denis Kirjanov wrote: >> The patch fixes the following clang warnings: >> >> warning: field 'xxx' with variable sized type 'xxx' not at the end of a struct >> or class is a GNU extension [-Wgnu-variable-sized-type-not-at-end] >> >> Signed-off-by: Denis Kirjanov <dkirjanov@suse.de> > > Have you checked if this does not break the ioctl interface? Many of > these structures (maybe even all of them) are directly passed to kernel > via ioctl so that rearranging them would break the data block format > expected by kernel. > > AFAICS at least in the cases that I checked, the outer struct member is > exactly the data expected as variable part of the inner struct. > > Michal Yes, it seems the correct solution is to just remove from struct ethtool_link_settings: __u32 link_mode_masks[]; This is unused in the driver, and the mask-handing command creates an ecmd which appends the masks However this is a UAPI struct, so this potentially breaks userspace compilation (but not the UABI). /jeff
diff --git a/ethtool.c b/ethtool.c index 3ac15a7..32e79ae 100644 --- a/ethtool.c +++ b/ethtool.c @@ -1736,8 +1736,8 @@ get_stringset(struct cmd_context *ctx, enum ethtool_stringset set_id, ptrdiff_t drvinfo_offset, int null_terminate) { struct { - struct ethtool_sset_info hdr; u32 buf[1]; + struct ethtool_sset_info hdr; } sset_info; struct ethtool_drvinfo drvinfo; u32 len, i; @@ -2683,8 +2683,8 @@ do_ioctl_glinksettings(struct cmd_context *ctx) { int err; struct { - struct ethtool_link_settings req; __u32 link_mode_data[3 * ETHTOOL_LINK_MODE_MASK_MAX_KERNEL_NU32]; + struct ethtool_link_settings req; } ecmd; struct ethtool_link_usettings *link_usettings; unsigned int u32_offs; @@ -2752,8 +2752,8 @@ do_ioctl_slinksettings(struct cmd_context *ctx, const struct ethtool_link_usettings *link_usettings) { struct { - struct ethtool_link_settings req; __u32 link_mode_data[3 * ETHTOOL_LINK_MODE_MASK_MAX_KERNEL_NU32]; + struct ethtool_link_settings req; } ecmd; unsigned int u32_offs; @@ -5206,8 +5206,8 @@ static int do_get_phy_tunable(struct cmd_context *ctx) if (!strcmp(argp[0], "downshift")) { struct { - struct ethtool_tunable ds; u8 count; + struct ethtool_tunable ds; } cont; cont.ds.cmd = ETHTOOL_PHY_GTUNABLE; @@ -5224,8 +5224,8 @@ static int do_get_phy_tunable(struct cmd_context *ctx) fprintf(stdout, "Downshift disabled\n"); } else if (!strcmp(argp[0], "fast-link-down")) { struct { - struct ethtool_tunable fld; u8 msecs; + struct ethtool_tunable fld; } cont; cont.fld.cmd = ETHTOOL_PHY_GTUNABLE; @@ -5246,8 +5246,8 @@ static int do_get_phy_tunable(struct cmd_context *ctx) cont.msecs); } else if (!strcmp(argp[0], "energy-detect-power-down")) { struct { - struct ethtool_tunable ds; u16 msecs; + struct ethtool_tunable ds; } cont; cont.ds.cmd = ETHTOOL_PHY_GTUNABLE; @@ -5494,8 +5494,8 @@ static int do_set_phy_tunable(struct cmd_context *ctx) /* Do it */ if (ds_changed) { struct { - struct ethtool_tunable ds; u8 count; + struct ethtool_tunable ds; } cont; cont.ds.cmd = ETHTOOL_PHY_STUNABLE; @@ -5510,8 +5510,8 @@ static int do_set_phy_tunable(struct cmd_context *ctx) } } else if (fld_changed) { struct { - struct ethtool_tunable fld; u8 msecs; + struct ethtool_tunable fld; } cont; cont.fld.cmd = ETHTOOL_PHY_STUNABLE; @@ -5526,8 +5526,8 @@ static int do_set_phy_tunable(struct cmd_context *ctx) } } else if (edpd_changed) { struct { - struct ethtool_tunable fld; u16 msecs; + struct ethtool_tunable fld; } cont; cont.fld.cmd = ETHTOOL_PHY_STUNABLE; diff --git a/internal.h b/internal.h index 4b994f5..e0beec6 100644 --- a/internal.h +++ b/internal.h @@ -152,12 +152,12 @@ struct ethtool_link_usettings { struct { __u8 transceiver; } deprecated; - struct ethtool_link_settings base; struct { ETHTOOL_DECLARE_LINK_MODE_MASK(supported); ETHTOOL_DECLARE_LINK_MODE_MASK(advertising); ETHTOOL_DECLARE_LINK_MODE_MASK(lp_advertising); } link_modes; + struct ethtool_link_settings base; }; #define ethtool_link_mode_for_each_u32(index) \
The patch fixes the following clang warnings: warning: field 'xxx' with variable sized type 'xxx' not at the end of a struct or class is a GNU extension [-Wgnu-variable-sized-type-not-at-end] Signed-off-by: Denis Kirjanov <dkirjanov@suse.de> --- ethtool.c | 18 +++++++++--------- internal.h | 2 +- 2 files changed, 10 insertions(+), 10 deletions(-)