| Message ID | 20220608191919.327705-2-mathew.j.martineau@linux.intel.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | mptcp: Header fixups | expand |
On Wed, 8 Jun 2022 12:19:18 -0700 Mat Martineau wrote: > From: Ossama Othman <ossama.othman@intel.com> > > Including <linux/mptcp.h> before the C library <netinet/in.h> header > causes symbol redefinition errors at compile-time due to duplicate > declarations and definitions in the <linux/in.h> header included by > <linux/mptcp.h>. > > Explicitly include <netinet/in.h> before <linux/in.h> in > <linux/mptcp.h> when __KERNEL__ is not defined so that the C library > compatibility logic in <linux/libc-compat.h> is enabled when including > <linux/mptcp.h> in user space code. > > Fixes: c11c5906bc0a ("mptcp: add MPTCP_SUBFLOW_ADDRS getsockopt support") What does it break, tho? The commit under Fixes is in net, if it's really a fix it needs to go to net. If it's just prep for another change we don't need to fixes tag.
On Thu, 9 Jun 2022, Jakub Kicinski wrote: > On Wed, 8 Jun 2022 12:19:18 -0700 Mat Martineau wrote: >> From: Ossama Othman <ossama.othman@intel.com> >> >> Including <linux/mptcp.h> before the C library <netinet/in.h> header >> causes symbol redefinition errors at compile-time due to duplicate >> declarations and definitions in the <linux/in.h> header included by >> <linux/mptcp.h>. >> >> Explicitly include <netinet/in.h> before <linux/in.h> in >> <linux/mptcp.h> when __KERNEL__ is not defined so that the C library >> compatibility logic in <linux/libc-compat.h> is enabled when including >> <linux/mptcp.h> in user space code. >> >> Fixes: c11c5906bc0a ("mptcp: add MPTCP_SUBFLOW_ADDRS getsockopt support") > > What does it break, tho? The commit under Fixes is in net, if it's > really a fix it needs to go to net. If it's just prep for another > change we don't need to fixes tag. > Hi Jakub - This is a minor "fix" to be sure, which I thought did not meet the bar for net and therefore submitted for net-next. It's not prep for another change, it's something Ossama and I noticed when doing code review for a userspace program that included the header. There's no problem with kernel compilation, and there's also no issue if the userspace program happens to include netinet/in.h before linux/mptcp.h If my threshold for the net branch is too high, I have no objection to having this patch applied there and will recalibrate :) Do you prefer to have no Fixes: tags in net-next, or did that just seem ambiguous in this case? -- Mat Martineau Intel
On Fri, 10 Jun 2022 11:00:28 -0700 (PDT) Mat Martineau wrote: > This is a minor "fix" to be sure, which I thought did not meet the bar for > net and therefore submitted for net-next. It's not prep for another > change, it's something Ossama and I noticed when doing code review for a > userspace program that included the header. There's no problem with kernel > compilation, and there's also no issue if the userspace program happens to > include netinet/in.h before linux/mptcp.h > > > If my threshold for the net branch is too high, I have no objection to > having this patch applied there and will recalibrate :) > > Do you prefer to have no Fixes: tags in net-next, or did that just seem > ambiguous in this case? The important point is that the middle ground of marking things as fixes and at the same time putting them in -next, to still get them backported but with an extended settling time -- that middle ground does not exist. If we look at the patch from the "do we want it backported or not" perspective I think the answer is yes, hence I'd lean towards net. If you think it doesn't matter enough for backport - we can drop the fixes tag and go with net-next. Unfortunately I don't have enough direct experience to tell how annoying this will be to the user space. netinet/in.h vs linux/in.h is a mess :(
On Fri, 10 Jun 2022, Jakub Kicinski wrote: > On Fri, 10 Jun 2022 11:00:28 -0700 (PDT) Mat Martineau wrote: >> This is a minor "fix" to be sure, which I thought did not meet the bar for >> net and therefore submitted for net-next. It's not prep for another >> change, it's something Ossama and I noticed when doing code review for a >> userspace program that included the header. There's no problem with kernel >> compilation, and there's also no issue if the userspace program happens to >> include netinet/in.h before linux/mptcp.h >> >> >> If my threshold for the net branch is too high, I have no objection to >> having this patch applied there and will recalibrate :) >> >> Do you prefer to have no Fixes: tags in net-next, or did that just seem >> ambiguous in this case? > > The important point is that the middle ground of marking things as fixes > and at the same time putting them in -next, to still get them > backported but with an extended settling time -- that middle ground > does not exist. > > If we look at the patch from the "do we want it backported or not" > perspective I think the answer is yes, hence I'd lean towards net. > If you think it doesn't matter enough for backport - we can drop the > fixes tag and go with net-next. Unfortunately I don't have enough > direct experience to tell how annoying this will be to the user space. > netinet/in.h vs linux/in.h is a mess :( > By that criteria, I lean towards net too. Thanks Jakub. -- Mat Martineau Intel
diff --git a/include/uapi/linux/mptcp.h b/include/uapi/linux/mptcp.h index 921963589904..dfe19bf13f4c 100644 --- a/include/uapi/linux/mptcp.h +++ b/include/uapi/linux/mptcp.h @@ -2,16 +2,17 @@ #ifndef _UAPI_MPTCP_H #define _UAPI_MPTCP_H +#ifndef __KERNEL__ +#include <netinet/in.h> /* for sockaddr_in and sockaddr_in6 */ +#include <sys/socket.h> /* for struct sockaddr */ +#endif + #include <linux/const.h> #include <linux/types.h> #include <linux/in.h> /* for sockaddr_in */ #include <linux/in6.h> /* for sockaddr_in6 */ #include <linux/socket.h> /* for sockaddr_storage and sa_family */ -#ifndef __KERNEL__ -#include <sys/socket.h> /* for struct sockaddr */ -#endif - #define MPTCP_SUBFLOW_FLAG_MCAP_REM _BITUL(0) #define MPTCP_SUBFLOW_FLAG_MCAP_LOC _BITUL(1) #define MPTCP_SUBFLOW_FLAG_JOIN_REM _BITUL(2)