| Message ID | 20230904162504.1356068-8-leitao@debian.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | io_uring: Initial support for {s,g}etsockopt commands | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Guessing tree name failed - patch did not apply, async |
| bpf/vmtest-bpf-PR | fail | merge-conflict |
Breno Leitao <leitao@debian.org> writes: > Protect io_uring_cmd_sock() to be called if CONFIG_NET is not set. If > network is not enabled, but io_uring is, then we want to return > -EOPNOTSUPP for any possible socket operation. > > This is helpful because io_uring_cmd_sock() can now call functions that > only exits if CONFIG_NET is enabled without having #ifdef CONFIG_NET > inside the function itself. > > Signed-off-by: Breno Leitao <leitao@debian.org> > --- > io_uring/uring_cmd.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/io_uring/uring_cmd.c b/io_uring/uring_cmd.c > index 60f843a357e0..6a91e1af7d05 100644 > --- a/io_uring/uring_cmd.c > +++ b/io_uring/uring_cmd.c > @@ -167,6 +167,7 @@ int io_uring_cmd_import_fixed(u64 ubuf, unsigned long len, int rw, > } > EXPORT_SYMBOL_GPL(io_uring_cmd_import_fixed); > > +#if defined(CONFIG_NET) > int io_uring_cmd_sock(struct io_uring_cmd *cmd, unsigned int issue_flags) > { > struct socket *sock = cmd->file->private_data; > @@ -192,4 +193,11 @@ int io_uring_cmd_sock(struct io_uring_cmd *cmd, unsigned int issue_flags) > return -EOPNOTSUPP; > } > } > +#else > +int io_uring_cmd_sock(struct io_uring_cmd *cmd, unsigned int issue_flags) > +{ > + return -EOPNOTSUPP; > +} > +#endif > + > EXPORT_SYMBOL_GPL(io_uring_cmd_sock); It doesn't make much sense to export the symbol on the !CONFIG_NET case. Usually, you'd make it a 'static inline' in the header file (even though it won't be ever inlined in this case): in include/linux/io_uring.h: #if defined(CONFIG_NET) int io_uring_cmd_sock(struct io_uring_cmd *cmd, unsigned int issue_flags); #else static inline int io_uring_cmd_sock(struct io_uring_cmd *cmd, unsigned int issue_flags) { return -EOPNOTSUPP; } #endif But this is a minor detail. I'd say to consider doing it if you end up doing another spin of the patchset. Other than that, looks good to me. Thanks,
On Tue, Sep 05, 2023 at 08:32:15AM -0400, Gabriel Krisman Bertazi wrote: > Breno Leitao <leitao@debian.org> writes: > > > Protect io_uring_cmd_sock() to be called if CONFIG_NET is not set. If > > network is not enabled, but io_uring is, then we want to return > > -EOPNOTSUPP for any possible socket operation. > > > > This is helpful because io_uring_cmd_sock() can now call functions that > > only exits if CONFIG_NET is enabled without having #ifdef CONFIG_NET > > inside the function itself. > > > > Signed-off-by: Breno Leitao <leitao@debian.org> > > --- > > io_uring/uring_cmd.c | 8 ++++++++ > > 1 file changed, 8 insertions(+) > > > > diff --git a/io_uring/uring_cmd.c b/io_uring/uring_cmd.c > > index 60f843a357e0..6a91e1af7d05 100644 > > --- a/io_uring/uring_cmd.c > > +++ b/io_uring/uring_cmd.c > > @@ -167,6 +167,7 @@ int io_uring_cmd_import_fixed(u64 ubuf, unsigned long len, int rw, > > } > > EXPORT_SYMBOL_GPL(io_uring_cmd_import_fixed); > > > > +#if defined(CONFIG_NET) > > int io_uring_cmd_sock(struct io_uring_cmd *cmd, unsigned int issue_flags) > > { > > struct socket *sock = cmd->file->private_data; > > @@ -192,4 +193,11 @@ int io_uring_cmd_sock(struct io_uring_cmd *cmd, unsigned int issue_flags) > > return -EOPNOTSUPP; > > } > > } > > +#else > > +int io_uring_cmd_sock(struct io_uring_cmd *cmd, unsigned int issue_flags) > > +{ > > + return -EOPNOTSUPP; > > +} > > +#endif > > + > > EXPORT_SYMBOL_GPL(io_uring_cmd_sock); > > It doesn't make much sense to export the symbol on the !CONFIG_NET case. > Usually, you'd make it a 'static inline' in the header file (even though > it won't be ever inlined in this case): > > in include/linux/io_uring.h: > > #if defined(CONFIG_NET) > int io_uring_cmd_sock(struct io_uring_cmd *cmd, unsigned int issue_flags); > #else > static inline int io_uring_cmd_sock(struct io_uring_cmd *cmd, unsigned int issue_flags) > { > return -EOPNOTSUPP; > } > #endif > > But this is a minor detail. I'd say to consider doing it if you end up doing > another spin of the patchset. Other than that, looks good to me. This makes sense, and I will add the symbol export inside the "if defined(CONFIG_NET)" block, since I need to respin this patchset to address the sockptr_t concern. Thanks for the good reviews!
diff --git a/io_uring/uring_cmd.c b/io_uring/uring_cmd.c index 60f843a357e0..6a91e1af7d05 100644 --- a/io_uring/uring_cmd.c +++ b/io_uring/uring_cmd.c @@ -167,6 +167,7 @@ int io_uring_cmd_import_fixed(u64 ubuf, unsigned long len, int rw, } EXPORT_SYMBOL_GPL(io_uring_cmd_import_fixed); +#if defined(CONFIG_NET) int io_uring_cmd_sock(struct io_uring_cmd *cmd, unsigned int issue_flags) { struct socket *sock = cmd->file->private_data; @@ -192,4 +193,11 @@ int io_uring_cmd_sock(struct io_uring_cmd *cmd, unsigned int issue_flags) return -EOPNOTSUPP; } } +#else +int io_uring_cmd_sock(struct io_uring_cmd *cmd, unsigned int issue_flags) +{ + return -EOPNOTSUPP; +} +#endif + EXPORT_SYMBOL_GPL(io_uring_cmd_sock);
Protect io_uring_cmd_sock() to be called if CONFIG_NET is not set. If network is not enabled, but io_uring is, then we want to return -EOPNOTSUPP for any possible socket operation. This is helpful because io_uring_cmd_sock() can now call functions that only exits if CONFIG_NET is enabled without having #ifdef CONFIG_NET inside the function itself. Signed-off-by: Breno Leitao <leitao@debian.org> --- io_uring/uring_cmd.c | 8 ++++++++ 1 file changed, 8 insertions(+)